Opened 11 years ago

Closed 9 years ago

#3473 closed bug (fixed)

Haiku does not recognise some multi session CDs

Reported by: bbjimmy Owned by: axeld
Priority: normal Milestone: R1
Component: File Systems/cdda Version: R1/Development
Keywords: corum session mixed-mode EnhancedCD Cc:
Blocked By: #6130 Blocking: #7020
Has a Patch: yes Platform: All

Description

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.

Attachments (4)

Corum-disc-syslog.txt (2.4 KB ) - added by Ziusudra 9 years ago.
R5-Corum-disc.png (2.9 KB ) - added by Ziusudra 9 years ago.
session.patch (667 bytes ) - added by Ziusudra 9 years ago.
mixed-mode-cd.patch (17.2 KB ) - added by Ziusudra 9 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 by axeld, 10 years ago

Component: - GeneralFile Systems/ISO 9660
Version: R1/pre-alpha1R1/Development

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.

by Ziusudra, 9 years ago

Attachment: Corum-disc-syslog.txt added

by Ziusudra, 9 years ago

Attachment: R5-Corum-disc.png added

comment:2 by Ziusudra, 9 years ago

Keywords: corum session added

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.

by Ziusudra, 9 years ago

Attachment: session.patch added

comment:3 by Ziusudra, 9 years ago

Has a Patch: set

comment:4 by Ziusudra, 9 years ago

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.

comment:5 by axeld, 9 years ago

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?)

comment:6 by Ziusudra, 9 years ago

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.

comment:7 by axeld, 9 years ago

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?

comment:8 by Ziusudra, 9 years ago

Summary: Haiku does not recognise multi session CDsHaiku does not recognise some multi session CDs

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.)

comment:9 by axeld, 9 years ago

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.

comment:10 by Ziusudra, 9 years ago

Blocked By: 6130 added

by Ziusudra, 9 years ago

Attachment: mixed-mode-cd.patch added

comment:11 by Ziusudra, 9 years ago

Component: File Systems/ISO 9660File Systems/cdda
Keywords: mixed-mode EnhancedCD added

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

comment:12 by axeld, 9 years ago

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.

comment:13 by Ziusudra, 9 years ago

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. :/

comment:14 by anevilyak, 9 years ago

Blocking: 7020 added

comment:15 by axeld, 9 years ago

Resolution: fixed
Status: newclosed

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!

Note: See TracTickets for help on using tickets.