[haiku-bugs] Re: [Haiku] #3473: Haiku does not recognise some multi session CDs
- From: "axeld" <trac@xxxxxxxxxxxx>
- Date: Sun, 13 Jun 2010 17:00:30 -0000
#3473: Haiku does not recognise some multi session CDs
-------------------------------------------------+--------------------------
Reporter: bbjimmy | Owner: axeld
Type: bug | Status: new
Priority: normal | Milestone: R1
Component: File Systems/cdda | Version: R1/Development
Keywords: corum session mixed-mode EnhancedCD | Blockedby: 6130
Patch: 1 | Platform: All
Blocking: |
-------------------------------------------------+--------------------------
Comment(by 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@xxxxxxxxxx
*/
-
/*! \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 r37126 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.
--
Ticket URL: <http://dev.haiku-os.org/ticket/3473#comment:12>
Haiku <http://dev.haiku-os.org>
Haiku - the operating system.
Other related posts:
