[Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Programmingkid]

[-- Attachment #1: Type: text/plain, Size: 1445 bytes --]

Fix real cdrom access in Mac OS X so it can be used in QEMU.
It simply removes the r from a device file's name. This
allows for a real cdrom to be accessible to the guest.
It has been successfully tested with a Windows XP guest
in qemu-system-i386. The qemu-system-ppc emulator doesn't
quit anymore, but there is another problem that prevents a 
real cdrom from working.

Signed-off-by: John Arbuckle <programmingkidx@gmail.com>

---
 block/raw-posix.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index a967464..3585ed9 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -2096,6 +2096,16 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
         kernResult = FindEjectableCDMedia( &mediaIterator );
         kernResult = GetBSDPath( mediaIterator, bsdPath, sizeof( bsdPath ) );
 
+        /*
+         * Remove the r from cdrom block device if needed.
+         * /dev/rdisk1 would become /dev/disk1.
+         * The r means raw access. It doesn't work well.
+         */
+        int sizeOfString = strlen("/dev/r");
+        if (strncmp("/dev/r", bsdPath, sizeOfString) == 0) {
+            sprintf(bsdPath, "/dev/%s", bsdPath + sizeOfString);
+        }
+
         if ( bsdPath[ 0 ] != '\0' ) {
             strcat(bsdPath,"s0");
             /* some CDs don't have a partition 0 */
-- 
1.7.5.4


[-- Attachment #2: Type: text/html, Size: 7306 bytes --]

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[M A]

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]


On Jul 1, 2015, at 6:13 PM, Programmingkid wrote:

> Fix real cdrom access in Mac OS X so it can be used in QEMU.
> It simply removes the r from a device file's name. This
> allows for a real cdrom to be accessible to the guest.
> It has been successfully tested with a Windows XP guest
> in qemu-system-i386. The qemu-system-ppc emulator doesn't
> quit anymore, but there is another problem that prevents a 
> real cdrom from working.

Just wanted to note that qemu-system-ppc does work. I was able to read
a file from a real cd from the Mac OS 10.2 guest. It was OpenBIOS that has
the problem. 


[-- Attachment #2: Type: text/html, Size: 1951 bytes --]

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Paolo Bonzini]

On 02/07/2015 00:13, Programmingkid wrote:
> Fix real cdrom access in Mac OS X so it can be used in QEMU.
> It simply removes the r from a device file's name. This
> allows for a real cdrom to be accessible to the guest.
> It has been successfully tested with a Windows XP guest
> in qemu-system-i386. The qemu-system-ppc emulator doesn't
> quit anymore, but there is another problem that prevents a 
> real cdrom from working.
> 
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com
> <mailto:programmingkidx@gmail.com>>
> 
> ---
>  block/raw-posix.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index a967464..3585ed9 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -2096,6 +2096,16 @@ static int hdev_open(BlockDriverState *bs, QDict
> *options, int flags,
>          kernResult = FindEjectableCDMedia( &mediaIterator );
>          kernResult = GetBSDPath( mediaIterator, bsdPath, sizeof(
> bsdPath ) );
> 
>  
> 
> +        /*
> +         * Remove the r from cdrom block device if needed.
> +         * /dev/rdisk1 would become /dev/disk1.
> +         * The r means raw access. It doesn't work well.
> +         */
> +        int sizeOfString = strlen("/dev/r");
> +        if (strncmp("/dev/r", bsdPath, sizeOfString) == 0) {
> +            sprintf(bsdPath, "/dev/%s", bsdPath + sizeOfString);
> +        }

I think you can just remove the strcat in here:

        if ( bsdPathAsCFString ) {
            size_t devPathLength;
            strcpy( bsdPath, _PATH_DEV );
            strcat( bsdPath, "r" );
            devPathLength = strlen( bsdPath );
            if ( CFStringGetCString( bsdPathAsCFString, bsdPath +
devPathLength, maxPathSize - devPathLength, kCFStringEncodingASCII ) ) {
                kernResult = KERN_SUCCESS;
            }
            CFRelease( bsdPathAsCFString );

So it's still a one-line change.  What are the other consequences of
removing the "r"?

Paolo

>          if ( bsdPath[ 0 ] != '\0' ) {
>              strcat(bsdPath,"s0");
>              /* some CDs don't have a partition 0 */
> -- 
> 1.7.5.4
> 

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Laurent Vivier]

On 02/07/2015 09:18, Paolo Bonzini wrote:
> 
> 
> On 02/07/2015 00:13, Programmingkid wrote:
>> Fix real cdrom access in Mac OS X so it can be used in QEMU.
>> It simply removes the r from a device file's name. This
>> allows for a real cdrom to be accessible to the guest.
>> It has been successfully tested with a Windows XP guest
>> in qemu-system-i386. The qemu-system-ppc emulator doesn't
>> quit anymore, but there is another problem that prevents a 
>> real cdrom from working.
>>
>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com
>> <mailto:programmingkidx@gmail.com>>
>>
>> ---
>>  block/raw-posix.c |   10 ++++++++++
>>  1 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index a967464..3585ed9 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -2096,6 +2096,16 @@ static int hdev_open(BlockDriverState *bs, QDict
>> *options, int flags,
>>          kernResult = FindEjectableCDMedia( &mediaIterator );
>>          kernResult = GetBSDPath( mediaIterator, bsdPath, sizeof(
>> bsdPath ) );
>>
>>  
>>
>> +        /*
>> +         * Remove the r from cdrom block device if needed.
>> +         * /dev/rdisk1 would become /dev/disk1.
>> +         * The r means raw access. It doesn't work well.
>> +         */
>> +        int sizeOfString = strlen("/dev/r");
>> +        if (strncmp("/dev/r", bsdPath, sizeOfString) == 0) {
>> +            sprintf(bsdPath, "/dev/%s", bsdPath + sizeOfString);
>> +        }
> 
> I think you can just remove the strcat in here:
> 
>         if ( bsdPathAsCFString ) {
>             size_t devPathLength;
>             strcpy( bsdPath, _PATH_DEV );
>             strcat( bsdPath, "r" );
>             devPathLength = strlen( bsdPath );
>             if ( CFStringGetCString( bsdPathAsCFString, bsdPath +
> devPathLength, maxPathSize - devPathLength, kCFStringEncodingASCII ) ) {
>                 kernResult = KERN_SUCCESS;
>             }
>             CFRelease( bsdPathAsCFString );
> 
> So it's still a one-line change.  What are the other consequences of
> removing the "r"?

This code seems to be a cut'n'paste of an Apple example (bad...):

https://developer.apple.com/library/mac/samplecode/CDROMSample/Listings/CDROMSample_CDROMSample_c.html

Without this interesting comment:

    Add "r" before the BSD node name from the I/O Registry to specify
    the raw disknode. The raw disk nodes receive I/O requests directly
    and do not go through the buffer cache.

Laurent

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Paolo Bonzini]

On 02/07/2015 09:39, Laurent Vivier wrote:
> This code seems to be a cut'n'paste of an Apple example (bad...):
> 
> https://developer.apple.com/library/mac/samplecode/CDROMSample/Listings/CDROMSample_CDROMSample_c.html
> 
> Without this interesting comment:
> 
>     Add "r" before the BSD node name from the I/O Registry to specify
>     the raw disknode. The raw disk nodes receive I/O requests directly
>     and do not go through the buffer cache.

Ok, so that's not too bad.

Paolo

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Stefan Hajnoczi]

On Wed, Jul 1, 2015 at 11:13 PM, Programmingkid
<programmingkidx@gmail.com> wrote:
> Fix real cdrom access in Mac OS X so it can be used in QEMU.
> It simply removes the r from a device file's name. This
> allows for a real cdrom to be accessible to the guest.
> It has been successfully tested with a Windows XP guest
> in qemu-system-i386. The qemu-system-ppc emulator doesn't
> quit anymore, but there is another problem that prevents a
> real cdrom from working.
>
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>
> ---
>  block/raw-posix.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index a967464..3585ed9 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -2096,6 +2096,16 @@ static int hdev_open(BlockDriverState *bs, QDict
> *options, int flags,
>          kernResult = FindEjectableCDMedia( &mediaIterator );
>          kernResult = GetBSDPath( mediaIterator, bsdPath, sizeof( bsdPath )
> );
>
>
>
> +        /*
> +         * Remove the r from cdrom block device if needed.
> +         * /dev/rdisk1 would become /dev/disk1.
> +         * The r means raw access. It doesn't work well.
> +         */
> +        int sizeOfString = strlen("/dev/r");
> +        if (strncmp("/dev/r", bsdPath, sizeOfString) == 0) {
> +            sprintf(bsdPath, "/dev/%s", bsdPath + sizeOfString);
> +        }

Why doesn't raw access "work well"?

This patch looks like a workaround but you haven't identified the root cause.

Perhaps because of request alignment requirements?  Typically CD-ROMs
have 2 KB block size.  You could test this by changing the following
in block_int.h:
#define BLOCK_PROBE_BUF_SIZE        512

In that case you need to fix raw-posix.c alignment detection code for
Mac OS X.  Look at raw_probe_alignment().

Stefan

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Laurent Vivier]

On 02/07/2015 13:14, Stefan Hajnoczi wrote:
> On Wed, Jul 1, 2015 at 11:13 PM, Programmingkid
> <programmingkidx@gmail.com> wrote:
>> Fix real cdrom access in Mac OS X so it can be used in QEMU.
>> It simply removes the r from a device file's name. This
>> allows for a real cdrom to be accessible to the guest.
>> It has been successfully tested with a Windows XP guest
>> in qemu-system-i386. The qemu-system-ppc emulator doesn't
>> quit anymore, but there is another problem that prevents a
>> real cdrom from working.
>>
>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>>
>> ---
>>  block/raw-posix.c |   10 ++++++++++
>>  1 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index a967464..3585ed9 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -2096,6 +2096,16 @@ static int hdev_open(BlockDriverState *bs, QDict
>> *options, int flags,
>>          kernResult = FindEjectableCDMedia( &mediaIterator );
>>          kernResult = GetBSDPath( mediaIterator, bsdPath, sizeof( bsdPath )
>> );
>>
>>
>>
>> +        /*
>> +         * Remove the r from cdrom block device if needed.
>> +         * /dev/rdisk1 would become /dev/disk1.
>> +         * The r means raw access. It doesn't work well.
>> +         */
>> +        int sizeOfString = strlen("/dev/r");
>> +        if (strncmp("/dev/r", bsdPath, sizeOfString) == 0) {
>> +            sprintf(bsdPath, "/dev/%s", bsdPath + sizeOfString);
>> +        }
> 
> Why doesn't raw access "work well"?
> 
> This patch looks like a workaround but you haven't identified the root cause.
> 
> Perhaps because of request alignment requirements?  Typically CD-ROMs
> have 2 KB block size.  You could test this by changing the following
> in block_int.h:
> #define BLOCK_PROBE_BUF_SIZE        512
> 
> In that case you need to fix raw-posix.c alignment detection code for
> Mac OS X.  Look at raw_probe_alignment().

I agree with that, I think we need "s->needs_alignment = true".

So in raw_open_common() change "#ifdef __FreeBSD__" by "#ifdef
CONFIG_BSD" in:

#ifdef __FreeBSD__
    if (S_ISCHR(st.st_mode)) {
        /*
         * The file is a char device (disk), which on FreeBSD isn't behind
         * a pager, so force all requests to be aligned. This is needed
         * so QEMU makes sure all IO operations on the device are aligned
         * to sector size, or else FreeBSD will reject them with EINVAL.
         */
        s->needs_alignment = true;
    }
#endif

Laurent

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Paolo Bonzini]

On 02/07/2015 14:24, Laurent Vivier wrote:
> 
> #ifdef __FreeBSD__
>     if (S_ISCHR(st.st_mode)) {
>         /*
>          * The file is a char device (disk), which on FreeBSD isn't behind
>          * a pager, so force all requests to be aligned. This is needed
>          * so QEMU makes sure all IO operations on the device are aligned
>          * to sector size, or else FreeBSD will reject them with EINVAL.
>          */
>         s->needs_alignment = true;
>     }
> #endif

So on FreeBSD and Apple /dev/r* is the equivalent of BDRV_O_NO_CACHE?

Paolo

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Laurent Vivier]

On 02/07/2015 15:47, Paolo Bonzini wrote:
> 
> 
> On 02/07/2015 14:24, Laurent Vivier wrote:
>>
>> #ifdef __FreeBSD__
>>     if (S_ISCHR(st.st_mode)) {
>>         /*
>>          * The file is a char device (disk), which on FreeBSD isn't behind
>>          * a pager, so force all requests to be aligned. This is needed
>>          * so QEMU makes sure all IO operations on the device are aligned
>>          * to sector size, or else FreeBSD will reject them with EINVAL.
>>          */
>>         s->needs_alignment = true;
>>     }
>> #endif
> 
> So on FreeBSD and Apple /dev/r* is the equivalent of BDRV_O_NO_CACHE?

This is what I understand (MacOS is a derivative from FreeBSD)

https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/hdiutil.1.html

DEVICE SPECIAL FILES

Since any /dev entry can be treated as a raw disk image, it is worth
noting which devices can be accessed when and how. /dev/rdisk nodes are
character-special devices, but are "raw" in the BSD sense and force
block-aligned I/O. They are closer to the physical disk than the buffer
cache. /dev/disk nodes, on the other hand, are buffered block-special
devices and are used primarily by the kernel's filesystem code.

Laurent

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Paolo Bonzini]

On 02/07/2015 15:58, Laurent Vivier wrote:
> Since any /dev entry can be treated as a raw disk image, it is worth
> noting which devices can be accessed when and how. /dev/rdisk nodes are
> character-special devices, but are "raw" in the BSD sense and force
> block-aligned I/O. They are closer to the physical disk than the buffer
> cache. /dev/disk nodes, on the other hand, are buffered block-special
> devices and are used primarily by the kernel's filesystem code.

So the right thing to do would not be just to set need_alignment, but to
probe it like we do on Linux for BDRV_O_NO_CACHE.

I'm okay with doing the simple thing, but it needs a comment for non-BSDers.

Paolo

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Laurent Vivier]

On 02/07/2015 16:03, Paolo Bonzini wrote:
> 
> 
> On 02/07/2015 15:58, Laurent Vivier wrote:
>> Since any /dev entry can be treated as a raw disk image, it is worth
>> noting which devices can be accessed when and how. /dev/rdisk nodes are
>> character-special devices, but are "raw" in the BSD sense and force
>> block-aligned I/O. They are closer to the physical disk than the buffer
>> cache. /dev/disk nodes, on the other hand, are buffered block-special
>> devices and are used primarily by the kernel's filesystem code.
> 
> So the right thing to do would not be just to set need_alignment, but to
> probe it like we do on Linux for BDRV_O_NO_CACHE.
> 
> I'm okay with doing the simple thing, but it needs a comment for non-BSDers.

So, what we have to do, in our case, for MacOS X cdrom, is something like:

... GetBSDPath ...
...
    if (flags & BDRV_O_NOCACHE) {
        strcat(bsdPath, "r");
    }
...

?

Laurent

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Paolo Bonzini]

On 02/07/2015 16:18, Laurent Vivier wrote:
>> > I'm okay with doing the simple thing, but it needs a comment for non-BSDers.
> So, what we have to do, in our case, for MacOS X cdrom, is something like:
> 
> ... GetBSDPath ...
> ...
>     if (flags & BDRV_O_NOCACHE) {
>         strcat(bsdPath, "r");
>     }
> ...
> 
> ?

Well, what to do with Mac OS X CD-ROM is another story...  Raw access
"seems not do work well" according to John, so we may have a comment
there explaining why we're not adding the "r".

A FIXME comment saying "we should probe for alignment here" would be
placed where you check S_ISCHR and set need_alignment to true.

Paolo

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Laurent Vivier]

On 02/07/2015 16:20, Paolo Bonzini wrote:
> 
> 
> On 02/07/2015 16:18, Laurent Vivier wrote:
>>>> I'm okay with doing the simple thing, but it needs a comment for non-BSDers.
>> So, what we have to do, in our case, for MacOS X cdrom, is something like:
>>
>> ... GetBSDPath ...
>> ...
>>     if (flags & BDRV_O_NOCACHE) {
>>         strcat(bsdPath, "r");
>>     }
>> ...
>>
>> ?
> 
> Well, what to do with Mac OS X CD-ROM is another story...  Raw access
> "seems not do work well" according to John, so we may have a comment
> there explaining why we're not adding the "r".

I think it doesn't work well because they need to be aligned, and
NOCACHE implies that (with BDRV_O_NOCACHE code will be self explicit :) )

raw_open_common()

    if ((bs->open_flags & BDRV_O_NOCACHE) != 0) {
        s->needs_alignment = true;
    }

and needs_alignment allows to probe alignment (raw_probe_alignment())

> A FIXME comment saying "we should probe for alignment here" would be
> placed where you check S_ISCHR and set need_alignment to true.

It is another case,

in the previous case (MacOS cdrom), user provides "-cdrom /dev/cdrom"
and QEMU extracts /dev/rdiskX (or now /dev/diskX) from the system DB.

In the FreeBSD case, user provides /dev/diskX or /dev/rdiskX, and QEMU
must know if it needs alignment or not. I don't think we need more
comment here.

Laurent

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Programmingkid]

On Jul 2, 2015, at 10:33 AM, Laurent Vivier wrote:

> 
> 
> On 02/07/2015 16:20, Paolo Bonzini wrote:
>> 
>> 
>> On 02/07/2015 16:18, Laurent Vivier wrote:
>>>>> I'm okay with doing the simple thing, but it needs a comment for non-BSDers.
>>> So, what we have to do, in our case, for MacOS X cdrom, is something like:
>>> 
>>> ... GetBSDPath ...
>>> ...
>>>    if (flags & BDRV_O_NOCACHE) {
>>>        strcat(bsdPath, "r");
>>>    }
>>> ...
>>> 
>>> ?
>> 
>> Well, what to do with Mac OS X CD-ROM is another story...  Raw access
>> "seems not do work well" according to John, so we may have a comment
>> there explaining why we're not adding the "r".
> 
> I think it doesn't work well because they need to be aligned, and
> NOCACHE implies that (with BDRV_O_NOCACHE code will be self explicit :) )
> 
> raw_open_common()
> 
>    if ((bs->open_flags & BDRV_O_NOCACHE) != 0) {
>        s->needs_alignment = true;
>    }
> 
> and needs_alignment allows to probe alignment (raw_probe_alignment())

Thank you very much for this. Raw cdrom access now works with this code. I
just had to modify it so it just sets s->needs_alignment to true without checking
the if condition. The if condition didn't work. 

Re: [Qemu-devel] [PATCH v2] raw-posix.c: remove raw device access for cdrom

[Programmingkid]

[-- Attachment #1: Type: text/plain, Size: 4918 bytes --]

Allows QEMU running on Mac OS X guest to use the real
CD-ROM drive.

This patch doesn't actually remove raw device access. I'm just
using the same name as the last patch.

Why should we test for /dev/disk1s0 when /dev/rdisk1s0 works?
By avoiding using the raw device we gain speed. When a read
takes place for the raw CD-ROM device, it has to start spinning
up if it went to sleep. This takes time to do. Plus there is 
that annoying spin up sound. With the non-raw access, QEMU
can use the operating system's buffers to do its reading. 
This means faster and quieter access. 

I have noticed that in order to use /dev/disk1s0, the CD-ROM
had to be unmounted (but not ejected) from the desktop. 
Disk Utility is what I used to accomplish this. 

Signed-off-by: John Arbuckle <programmingkidx@gmail.com>

---
Tries more permutations of the CD-ROM device file.
Sets the needs_alignment variable.
Removes the unused kernResult variable.

 block/raw-posix.c |   62 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index cbe6574..2088c1f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -484,6 +484,19 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
 
     filename = qemu_opt_get(opts, "filename");
 
+#ifdef __APPLE__
+
+    /*
+     * Raw CD-ROM access on a Macintosh needs to be aligned
+     * or reads will fail.
+     */
+
+    if (strncmp(filename, "/dev/r", 6) == 0) {
+        s->needs_alignment = true;
+    }
+
+#endif  /* __APPLE__ */
+
     ret = raw_normalize_devicepath(&filename);
     if (ret != 0) {
         error_setg_errno(errp, -ret, "Could not normalize device path");
@@ -2014,7 +2027,6 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex ma
         if ( bsdPathAsCFString ) {
             size_t devPathLength;
             strcpy( bsdPath, _PATH_DEV );
-            strcat( bsdPath, "r" );
             devPathLength = strlen( bsdPath );
             if ( CFStringGetCString( bsdPathAsCFString, bsdPath + devPathLength, maxPathSize - devPathLength, kCFStringEncodingASCII ) ) {
                 kernResult = KERN_SUCCESS;
@@ -2120,25 +2132,55 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
     const char *filename = qdict_get_str(options, "filename");
 
     if (strstart(filename, "/dev/cdrom", NULL)) {
-        kern_return_t kernResult;
         io_iterator_t mediaIterator;
         char bsdPath[ MAXPATHLEN ];
         int fd;
 
-        kernResult = FindEjectableCDMedia( &mediaIterator );
-        kernResult = GetBSDPath( mediaIterator, bsdPath, sizeof( bsdPath ) );
+        FindEjectableCDMedia(&mediaIterator);
+        GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath));
 
         if ( bsdPath[ 0 ] != '\0' ) {
-            strcat(bsdPath,"s0");
-            /* some CDs don't have a partition 0 */
-            fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
-            if (fd < 0) {
-                bsdPath[strlen(bsdPath)-1] = '1';
-            } else {
+            int devStringLength = strlen("/dev/");
+            char baseDeviceName[MAXPATHLEN];
+            int numberOfOptions = 4;
+            char deviceNameArray[numberOfOptions][MAXPATHLEN];
+            int i = 0;
+
+            /* remove the "/dev/" part from the device file's name */
+            strcpy(baseDeviceName, bsdPath + devStringLength);
+
+            /* /dev/disk*s0 */
+            sprintf(deviceNameArray[i++], "/dev/%ss0", baseDeviceName);
+
+            /* /dev/disk*s1 */
+            sprintf(deviceNameArray[i++], "/dev/%ss1", baseDeviceName);
+
+            /* /dev/rdisk*s0 */
+            sprintf(deviceNameArray[i++], "/dev/r%ss0", baseDeviceName);
+
+            /* /dev/rdisk*s1 */
+            sprintf(deviceNameArray[i++], "/dev/r%ss1", baseDeviceName);
+
+            /* Try device file permutions until one works */
+            for (i = 0; i < numberOfOptions; i++) {
+                fd = qemu_open(deviceNameArray[i], O_RDONLY | O_BINARY
+                                                            | O_LARGEFILE);
+                if (fd < 0) {
+                    DPRINTF("Error opening %s: %s\n", deviceNameArray[i]
+                                                    , strerror(errno));
+                } else {
+                    strcpy(bsdPath, (char *)deviceNameArray[i]);
+                    break;
+                }
+            }
+
+            if (fd > 0) {
                 qemu_close(fd);
             }
+
             filename = bsdPath;
             qdict_put(options, "filename", qstring_from_str(filename));
+            DPRINTF("cdrom is using %s\n", filename);
         }
 
         if ( mediaIterator )
-- 
1.7.5.4


[-- Attachment #2: Type: text/html, Size: 25078 bytes --]

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Kevin Wolf]

Am 02.07.2015 um 16:18 hat Laurent Vivier geschrieben:
> 
> 
> On 02/07/2015 16:03, Paolo Bonzini wrote:
> > 
> > 
> > On 02/07/2015 15:58, Laurent Vivier wrote:
> >> Since any /dev entry can be treated as a raw disk image, it is worth
> >> noting which devices can be accessed when and how. /dev/rdisk nodes are
> >> character-special devices, but are "raw" in the BSD sense and force
> >> block-aligned I/O. They are closer to the physical disk than the buffer
> >> cache. /dev/disk nodes, on the other hand, are buffered block-special
> >> devices and are used primarily by the kernel's filesystem code.
> > 
> > So the right thing to do would not be just to set need_alignment, but to
> > probe it like we do on Linux for BDRV_O_NO_CACHE.
> > 
> > I'm okay with doing the simple thing, but it needs a comment for non-BSDers.
> 
> So, what we have to do, in our case, for MacOS X cdrom, is something like:
> 
> ... GetBSDPath ...
> ...
>     if (flags & BDRV_O_NOCACHE) {
>         strcat(bsdPath, "r");
>     }
> ...

I would avoid such magic. What we could do is rejecting /dev/rdisk nodes
without BDRV_O_NOCACHE.

Kevin

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Laurent Vivier]

On 08/07/2015 12:31, Kevin Wolf wrote:
> Am 02.07.2015 um 16:18 hat Laurent Vivier geschrieben:
>>
>>
>> On 02/07/2015 16:03, Paolo Bonzini wrote:
>>>
>>>
>>> On 02/07/2015 15:58, Laurent Vivier wrote:
>>>> Since any /dev entry can be treated as a raw disk image, it is worth
>>>> noting which devices can be accessed when and how. /dev/rdisk nodes are
>>>> character-special devices, but are "raw" in the BSD sense and force
>>>> block-aligned I/O. They are closer to the physical disk than the buffer
>>>> cache. /dev/disk nodes, on the other hand, are buffered block-special
>>>> devices and are used primarily by the kernel's filesystem code.
>>>
>>> So the right thing to do would not be just to set need_alignment, but to
>>> probe it like we do on Linux for BDRV_O_NO_CACHE.
>>>
>>> I'm okay with doing the simple thing, but it needs a comment for non-BSDers.
>>
>> So, what we have to do, in our case, for MacOS X cdrom, is something like:
>>
>> ... GetBSDPath ...
>> ...
>>     if (flags & BDRV_O_NOCACHE) {
>>         strcat(bsdPath, "r");
>>     }
>> ...
> 
> I would avoid such magic. What we could do is rejecting /dev/rdisk nodes
> without BDRV_O_NOCACHE.

It's not how it works...

Look in hdev_open().

If user provides /dev/cdrom on the command line, in the case of MacOS X,
QEMU searches for a cdrom drive in the system and set filename to
/dev/rdiskX according to the result.

Perhaps this part should be removed.

But if we just want to correct the bug, we must not set filename to
/dev/rdiskX if NOCACHE is not set but to /dev/diskX

It's the aim of this change.

Laurent

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Kevin Wolf]

Am 08.07.2015 um 12:47 hat Laurent Vivier geschrieben:
> 
> 
> On 08/07/2015 12:31, Kevin Wolf wrote:
> > Am 02.07.2015 um 16:18 hat Laurent Vivier geschrieben:
> >>
> >>
> >> On 02/07/2015 16:03, Paolo Bonzini wrote:
> >>>
> >>>
> >>> On 02/07/2015 15:58, Laurent Vivier wrote:
> >>>> Since any /dev entry can be treated as a raw disk image, it is worth
> >>>> noting which devices can be accessed when and how. /dev/rdisk nodes are
> >>>> character-special devices, but are "raw" in the BSD sense and force
> >>>> block-aligned I/O. They are closer to the physical disk than the buffer
> >>>> cache. /dev/disk nodes, on the other hand, are buffered block-special
> >>>> devices and are used primarily by the kernel's filesystem code.
> >>>
> >>> So the right thing to do would not be just to set need_alignment, but to
> >>> probe it like we do on Linux for BDRV_O_NO_CACHE.
> >>>
> >>> I'm okay with doing the simple thing, but it needs a comment for non-BSDers.
> >>
> >> So, what we have to do, in our case, for MacOS X cdrom, is something like:
> >>
> >> ... GetBSDPath ...
> >> ...
> >>     if (flags & BDRV_O_NOCACHE) {
> >>         strcat(bsdPath, "r");
> >>     }
> >> ...
> > 
> > I would avoid such magic. What we could do is rejecting /dev/rdisk nodes
> > without BDRV_O_NOCACHE.
> 
> It's not how it works...
> 
> Look in hdev_open().
> 
> If user provides /dev/cdrom on the command line, in the case of MacOS X,
> QEMU searches for a cdrom drive in the system and set filename to
> /dev/rdiskX according to the result.

Oh, we're already playing such games... I guess you're right then.

It even seems to be not only for '/dev/cdrom', but for everything
starting with this string. Does anyone know what's the reason for that?

Also, I guess before doing strcat() on bsdPath, we should check the
buffer length...

> Perhaps this part should be removed.
> 
> But if we just want to correct the bug, we must not set filename to
> /dev/rdiskX if NOCACHE is not set but to /dev/diskX
> 
> It's the aim of this change.

Yes, that looks right.

Kevin

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Programmingkid]

On Jul 8, 2015, at 7:01 AM, Kevin Wolf wrote:

> Am 08.07.2015 um 12:47 hat Laurent Vivier geschrieben:
>> 
>> 
>> On 08/07/2015 12:31, Kevin Wolf wrote:
>>> Am 02.07.2015 um 16:18 hat Laurent Vivier geschrieben:
>>>> 
>>>> 
>>>> On 02/07/2015 16:03, Paolo Bonzini wrote:
>>>>> 
>>>>> 
>>>>> On 02/07/2015 15:58, Laurent Vivier wrote:
>>>>>> Since any /dev entry can be treated as a raw disk image, it is worth
>>>>>> noting which devices can be accessed when and how. /dev/rdisk nodes are
>>>>>> character-special devices, but are "raw" in the BSD sense and force
>>>>>> block-aligned I/O. They are closer to the physical disk than the buffer
>>>>>> cache. /dev/disk nodes, on the other hand, are buffered block-special
>>>>>> devices and are used primarily by the kernel's filesystem code.
>>>>> 
>>>>> So the right thing to do would not be just to set need_alignment, but to
>>>>> probe it like we do on Linux for BDRV_O_NO_CACHE.
>>>>> 
>>>>> I'm okay with doing the simple thing, but it needs a comment for non-BSDers.
>>>> 
>>>> So, what we have to do, in our case, for MacOS X cdrom, is something like:
>>>> 
>>>> ... GetBSDPath ...
>>>> ...
>>>>    if (flags & BDRV_O_NOCACHE) {
>>>>        strcat(bsdPath, "r");
>>>>    }
>>>> ...
>>> 
>>> I would avoid such magic. What we could do is rejecting /dev/rdisk nodes
>>> without BDRV_O_NOCACHE.
>> 
>> It's not how it works...
>> 
>> Look in hdev_open().
>> 
>> If user provides /dev/cdrom on the command line, in the case of MacOS X,
>> QEMU searches for a cdrom drive in the system and set filename to
>> /dev/rdiskX according to the result.
> 
> Oh, we're already playing such games... I guess you're right then.
> 
> It even seems to be not only for '/dev/cdrom', but for everything
> starting with this string. Does anyone know what's the reason for that?
> 
> Also, I guess before doing strcat() on bsdPath, we should check the
> buffer length...

By buffer, do you mean the bsdPath variable?

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Kevin Wolf]

Am 08.07.2015 um 14:56 hat Programmingkid geschrieben:
> 
> On Jul 8, 2015, at 7:01 AM, Kevin Wolf wrote:
> 
> > Am 08.07.2015 um 12:47 hat Laurent Vivier geschrieben:
> >> 
> >> 
> >> On 08/07/2015 12:31, Kevin Wolf wrote:
> >>> Am 02.07.2015 um 16:18 hat Laurent Vivier geschrieben:
> >>>> 
> >>>> 
> >>>> On 02/07/2015 16:03, Paolo Bonzini wrote:
> >>>>> 
> >>>>> 
> >>>>> On 02/07/2015 15:58, Laurent Vivier wrote:
> >>>>>> Since any /dev entry can be treated as a raw disk image, it is worth
> >>>>>> noting which devices can be accessed when and how. /dev/rdisk nodes are
> >>>>>> character-special devices, but are "raw" in the BSD sense and force
> >>>>>> block-aligned I/O. They are closer to the physical disk than the buffer
> >>>>>> cache. /dev/disk nodes, on the other hand, are buffered block-special
> >>>>>> devices and are used primarily by the kernel's filesystem code.
> >>>>> 
> >>>>> So the right thing to do would not be just to set need_alignment, but to
> >>>>> probe it like we do on Linux for BDRV_O_NO_CACHE.
> >>>>> 
> >>>>> I'm okay with doing the simple thing, but it needs a comment for non-BSDers.
> >>>> 
> >>>> So, what we have to do, in our case, for MacOS X cdrom, is something like:
> >>>> 
> >>>> ... GetBSDPath ...
> >>>> ...
> >>>>    if (flags & BDRV_O_NOCACHE) {
> >>>>        strcat(bsdPath, "r");
> >>>>    }
> >>>> ...
> >>> 
> >>> I would avoid such magic. What we could do is rejecting /dev/rdisk nodes
> >>> without BDRV_O_NOCACHE.
> >> 
> >> It's not how it works...
> >> 
> >> Look in hdev_open().
> >> 
> >> If user provides /dev/cdrom on the command line, in the case of MacOS X,
> >> QEMU searches for a cdrom drive in the system and set filename to
> >> /dev/rdiskX according to the result.
> > 
> > Oh, we're already playing such games... I guess you're right then.
> > 
> > It even seems to be not only for '/dev/cdrom', but for everything
> > starting with this string. Does anyone know what's the reason for that?
> > 
> > Also, I guess before doing strcat() on bsdPath, we should check the
> > buffer length...
> 
> By buffer, do you mean the bsdPath variable?

Yes. In theory, bsdPath could be completely filled with the path
returned by GetBSDPath() because we pass sizeof(bsdPath) as maxPathSize.
Appending "s0" would then overflow the buffer.

I'll admit that this is rather unlikely to happen, but being careful
when dealing with strings can never hurt.

Kevin

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Programmingkid]

On Jul 8, 2015, at 9:11 AM, Kevin Wolf wrote:

> Am 08.07.2015 um 14:56 hat Programmingkid geschrieben:
>> 
>> On Jul 8, 2015, at 7:01 AM, Kevin Wolf wrote:
>> 
>>> Am 08.07.2015 um 12:47 hat Laurent Vivier geschrieben:
>>>> 
>>>> 
>>>> On 08/07/2015 12:31, Kevin Wolf wrote:
>>>>> Am 02.07.2015 um 16:18 hat Laurent Vivier geschrieben:
>>>>>> 
>>>>>> 
>>>>>> On 02/07/2015 16:03, Paolo Bonzini wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> On 02/07/2015 15:58, Laurent Vivier wrote:
>>>>>>>> Since any /dev entry can be treated as a raw disk image, it is worth
>>>>>>>> noting which devices can be accessed when and how. /dev/rdisk nodes are
>>>>>>>> character-special devices, but are "raw" in the BSD sense and force
>>>>>>>> block-aligned I/O. They are closer to the physical disk than the buffer
>>>>>>>> cache. /dev/disk nodes, on the other hand, are buffered block-special
>>>>>>>> devices and are used primarily by the kernel's filesystem code.
>>>>>>> 
>>>>>>> So the right thing to do would not be just to set need_alignment, but to
>>>>>>> probe it like we do on Linux for BDRV_O_NO_CACHE.
>>>>>>> 
>>>>>>> I'm okay with doing the simple thing, but it needs a comment for non-BSDers.
>>>>>> 
>>>>>> So, what we have to do, in our case, for MacOS X cdrom, is something like:
>>>>>> 
>>>>>> ... GetBSDPath ...
>>>>>> ...
>>>>>>   if (flags & BDRV_O_NOCACHE) {
>>>>>>       strcat(bsdPath, "r");
>>>>>>   }
>>>>>> ...
>>>>> 
>>>>> I would avoid such magic. What we could do is rejecting /dev/rdisk nodes
>>>>> without BDRV_O_NOCACHE.
>>>> 
>>>> It's not how it works...
>>>> 
>>>> Look in hdev_open().
>>>> 
>>>> If user provides /dev/cdrom on the command line, in the case of MacOS X,
>>>> QEMU searches for a cdrom drive in the system and set filename to
>>>> /dev/rdiskX according to the result.
>>> 
>>> Oh, we're already playing such games... I guess you're right then.
>>> 
>>> It even seems to be not only for '/dev/cdrom', but for everything
>>> starting with this string. Does anyone know what's the reason for that?
>>> 
>>> Also, I guess before doing strcat() on bsdPath, we should check the
>>> buffer length...
>> 
>> By buffer, do you mean the bsdPath variable?
> 
> Yes. In theory, bsdPath could be completely filled with the path
> returned by GetBSDPath() because we pass sizeof(bsdPath) as maxPathSize.
> Appending "s0" would then overflow the buffer.
> 
> I'll admit that this is rather unlikely to happen, but being careful
> when dealing with strings can never hurt.
> 
> Kevin

Ok, after my newest patch has been reviewed, I will add the size checking code. 

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Laurent Vivier]

On 08/07/2015 13:01, Kevin Wolf wrote:
> Am 08.07.2015 um 12:47 hat Laurent Vivier geschrieben:
>>
>>
>> On 08/07/2015 12:31, Kevin Wolf wrote:
>>> Am 02.07.2015 um 16:18 hat Laurent Vivier geschrieben:
>>>>
>>>>
>>>> On 02/07/2015 16:03, Paolo Bonzini wrote:
>>>>>
>>>>>
>>>>> On 02/07/2015 15:58, Laurent Vivier wrote:
>>>>>> Since any /dev entry can be treated as a raw disk image, it is worth
>>>>>> noting which devices can be accessed when and how. /dev/rdisk nodes are
>>>>>> character-special devices, but are "raw" in the BSD sense and force
>>>>>> block-aligned I/O. They are closer to the physical disk than the buffer
>>>>>> cache. /dev/disk nodes, on the other hand, are buffered block-special
>>>>>> devices and are used primarily by the kernel's filesystem code.
>>>>>
>>>>> So the right thing to do would not be just to set need_alignment, but to
>>>>> probe it like we do on Linux for BDRV_O_NO_CACHE.
>>>>>
>>>>> I'm okay with doing the simple thing, but it needs a comment for non-BSDers.
>>>>
>>>> So, what we have to do, in our case, for MacOS X cdrom, is something like:
>>>>
>>>> ... GetBSDPath ...
>>>> ...
>>>>     if (flags & BDRV_O_NOCACHE) {
>>>>         strcat(bsdPath, "r");
>>>>     }
>>>> ...
>>>
>>> I would avoid such magic. What we could do is rejecting /dev/rdisk nodes
>>> without BDRV_O_NOCACHE.
>>
>> It's not how it works...
>>
>> Look in hdev_open().
>>
>> If user provides /dev/cdrom on the command line, in the case of MacOS X,
>> QEMU searches for a cdrom drive in the system and set filename to
>> /dev/rdiskX according to the result.
> 
> Oh, we're already playing such games... I guess you're right then.
> 
> It even seems to be not only for '/dev/cdrom', but for everything
> starting with this string. Does anyone know what's the reason for that?


At least 10 years old reason:

commit 3b0d4f61c917c4612b561d75b33a11f4da00738b
Author: bellard <bellard@c046a42c-6fe2-441c-8c8c-71466251a162>
Date:   Sun Oct 30 18:30:10 2005 +0000

    OS X: support for the built in CD-ROM drive (Mike Kronenberg)




> 
> Also, I guess before doing strcat() on bsdPath, we should check the
> buffer length...
> 
>> Perhaps this part should be removed.
>>
>> But if we just want to correct the bug, we must not set filename to
>> /dev/rdiskX if NOCACHE is not set but to /dev/diskX
>>
>> It's the aim of this change.
> 
> Yes, that looks right.
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH] raw-posix.c: remove raw device access for cdrom

[Stefan Hajnoczi]

On Mon, Jul 6, 2015 at 4:58 PM, Programmingkid
<programmingkidx@gmail.com> wrote:
> Quick question, In order to use a real cdrom in buffered mode (/dev/disk1s0), QEMU would have to unmount the cdrom from the desktop. Is unmounting the cdrom in the hdev_open() function ok? . I am making a version 3 of the cdrom patch, so please disregard the last patch.

Please keep qemu-devel@nongnu.org CCed so discussion stays on the
mailing list and others can participate.

Does the user need to manually mount the CD-ROM again after QEMU has
terminated?  If so, then maybe the user should manually unmount before
running QEMU.

Stefan