Haiku does not recognise some multi session CDs

[bbjimmy]

If the CD has more than one session Haiku only recognizes the first session ... insert the corum111 CD and Haiku only recognizes the first session ... "Corum 111 - x86" not the "Corum 111" session ... the link to the data files is broken.

This may also be the problem installing Gobe Productive from the install CD as is is also multi session, but in this case neither BeOS hrev5 nor Zeta can mount the second session.

[axeld]

This is a combined issue of several components. I'll point it to the iso9660 file system first, as that would be the starting point.

[Ziusudra]

Here's a Haiku syslog and a pic of what R5 DriveSetup sees.

I'm guessing that bfs returning 0.8 before session returns 0.699 is the issue. So iso9660 is never is asked to try the rest of the disc.

Shouldn't all the partitioning systems get a go at it before any of the file systems or return a higher value?

The following works to install all the game files:

dd if=/dev/disk/atapi/0/slave/raw of=CorumIII-data.image bs=32K skip=1299
mkdir /Corum\ III
mount -ro CorumIII-data.image /Corum\ III

so iso9660 should recognise that session if given the chance.

Haiku does recognize the R4, R4.5 and R5 discs correctly but for those the iso9660 session comes before the bfs session and iso9660 returns 0.6.

[Ziusudra]

This patch changes session to act like the intel partitioning_system by returning 0.81 when there is more than one session and 0.5 when there is only one.

This fixes this issue with both the Gobe Productive and Corum III discs. Other already working multi-session disc are not affected.

[axeld]

I think I understand why you would want to override BFS, but I don't understand the reasoning for using 0.5 if there is only one session.

Unlike the intel partitioning, session data is not part of the data stream - the CD drive returns those via special commands. Therefore, I don't see how they could really be wrong, or does it have any negative effect if you always return 0.81? (I think I would even make it a 1.0, because what could be more significant?)

[Ziusudra]

The only reason I can think of for not always returning a high value is to avoid calling session's scan_partition if we don't have to.

[axeld]

Okay, the difference will be that there is only a raw device and no partitions - that seems to make some sense.

Anyway, I have the feeling that 0.81 might interfere with audio CDs - have you tested this?

[Ziusudra]

I have tested audio CDs and it does not interfere. session actually indentifies audio CD's and puts all the tracks in a single session. So, as far as its concerned, audio CDs have only one session and it returns 0.5. I guess that would be another reason not to always return high.

The only mixed mode CDs I can find are the old style with data as an audio track. The data is not seen as such but that is a bug with the session Disc::GetSession() that I am looking into fixing. (It doesn't check if the audio tracks are actually data.)

[axeld]

Thanks for the quick update. Of course, the interesting case would be one where both session, and cdda would claim ownership of the CD.

Since you already looking into fixing this, would you mind to postpone applying this patch until those CDs are recognized correctly? I assume it will be necessary to play a bit with this, anyway, then.

Anyway, thanks for working on all this!

If you want, you could also change session to use our private SCSI headers (for example scsi_cmds.h in private/drivers) instead of duplicating everything.

[Ziusudra]

This patch does the following:

• session now exposes all sessions of multi-session data CDs (Blue and Yellow Book)
• session now exposes all sessions of mixed-mode CDs (Blue and Yellow/Red Book)
• cdda now only claims audio sessions/CDs (session will override for mixed-mode CDs)
• single session audio or data CDs are unaffected
• fixes a couple of broken debug traces in cdda/kernel_interface
• clarifies a couple of local variable names to better reflect purpose (trackCount)
• style and automatic white space cleanup

[axeld]

Thanks! The patch looks fine for the most part, I just find the changes to CDDA a bit unclear/hard to follow. Am I correct that mixed mode CDs are no longer detected by CDDA, and require manual mounting? That sounds like something we should rather solve differently. Also, since it still would be a viable solution, reporting a lower value sounds like a better solution than returning an error.

Shouldn't CDDA only ever care about the current session, and should still work if called within a CDDA session? It just shouldn't override session in this case.

I also have a number of minor coding style issues; overall it looks already pretty good.

• src/add-ons/kernel/partitioning_systems/session/Disc.cpp:

  +++ src/add-ons/kernel/partitioning_systems/session/Disc.cpp	(working copy)
  @@ -6,7 +6,6 @@
    *		Tyler Akidau, haiku@akidau.net
    */
   
  -
   /*!	\file Disc.cpp
   
   	Disc class implementation, used to enumerate the CD/DVD sessions.
  @@ -21,7 +20,6 @@
   	number \c 0x2.
   */
   
  -
   #include "Disc.h"
  

There are two blank lines by intention as outlined in our style guide; if you are not yet used to our style, you shouldn't make any style changes, at least not as part of another patch.

+	if ((sessionIsAudio == false) || (foundCount != 1))

Superfluous parenthesis should be avoided.

+			if (entries[i].control & kControlDataTrack) {

You should always write (... & ...) != 0 instead - I just fixed a bug caused by ignoring this in hrev37126 which was a line you didn't change in your patch :-)

I saw that you worked around this issue at another case with extra () around the term, however, it's much less error prone to get used to using the form suggested in our coding style guide.

• src/add-ons/kernel/partitioning_systems/session/session.cpp:

  @@ -18,7 +18,6 @@
   #include "Debug.h"
   #include "Disc.h"
   
  -
   #define SESSION_PARTITION_MODULE_NAME "partitioning_systems/session/v1"
   
   
  @@ -46,13 +45,18 @@
   	device_geometry geometry;
   	float result = -1;
   	if (partition->flags & B_PARTITION_IS_DEVICE
  -		&& partition->block_size == 2048
  -		&& ioctl(fd, B_GET_GEOMETRY, &geometry) == 0
  -		&& geometry.device_type == B_CD) {
  +			&& partition->block_size == 2048
  +			&& ioctl(fd, B_GET_GEOMETRY, &geometry) == 0
  +			&& geometry.device_type == B_CD) {
  

While you did not fix the bug I noticed thanks to your patch, you made several style changes to code that actually followed our coding style before (and now no longer does).

• src/add-ons/kernel/file_systems/cdda/kernel_interface.cpp:

   				if (cookie->current == attribute)
  -					cookie->current = attribute->GetDoublyLinkedListLink()->next;
  +					cookie->current
  +						= attribute->GetDoublyLinkedListLink()->next;
  

The style change itself is correct as the code was violating the 80 character per line rule, but you also introduced a new violation, as when more than one line is enclosed by a if/for/..., it should always be enclosed by {}, no matter if your C compiler demands this.

@@ -1379,7 +1411,7 @@
 	}
 
 	*_cookie = toc;
-	return 0.8f;
+	return 0.8;

The return type is actually float, so 0.8f is correct.

-		// TODO: move that to open() to make sure reading can't fail for this reason?
+		// TODO: move to open() to insure reading can't fail for this reason?

typo: it's ensure, not insure.

In general, it's always a good idea to separate style changes from other changes, and don't put them into a single patch.

Anyway, thanks! I haven't yet applied your patch because of the identifying stuff, but as that's only nitpicking and your patch already improves the current situation, I could do so if you prefer.

[Ziusudra]

Am I correct that mixed mode CDs are no longer detected by CDDA, and require manual mounting?

No, CDDA will claim and automount the audio session for both types of mixed-mode CD. Other filesystem drivers will correctly claim and automount the data session.

Also, since it still would be a viable solution, reporting a lower value sounds like a better solution than returning an error.

No. If CDDA claims low for a data session and no other filesystem driver claims it, then CDDA will automount both sessions. Both will appear the same, as the audio session, and access to what is really the data session will panic to KDL. I have an EnhancedCD where the data session is HFS, for which we have no driver, where this happens if CDDA claims low.

You should always write (... & ...) != 0

OK, session::is_audio() in Disc.cpp will need to be fixed. Does that also apply to the short form that appears throughout the debug TRACEs?:

(entries[i].control & kControlDataTrack ? "data" : "audio")

I saw that you worked around this issue at another case with extra () around the term

Actually, I have a bad habit of using too many parathesis that I need to break. :/

[axeld]

After eleven months, I guess it's safe to say that you don't intend to update and clean your patch :-)

I've just come across this problem again, and I was happy to see that this patch solves a part of it! I've applied a cleaned up version of your patch in hrev41288, and tested it with different scenarios, which work fine now with respect to the changes made. Thanks a lot!