[Qemu-devel] [PATCH v6] block/raw-posix.c: Fixes raw_getlength() on Mac OS X so that it reports the correct length of a real CD

[Qemu-devel] [PATCH v6] block/raw-posix.c: Fixes raw_getlength() on Mac OS X so that it reports the correct length of a real CD

[Programmingkid]

Allows QEMU on Mac OS X to use a real cdrom again.

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

---
Added fallback code - uses lseek() if ioctl() fails.

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

diff --git a/block/raw-posix.c b/block/raw-posix.c
index e51293a..5815707 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1312,7 +1312,30 @@ again:
         if (size == 0)
 #endif
 #if defined(__APPLE__) && defined(__MACH__)
-        size = LLONG_MAX;
+    {
+        uint64_t sectors = 0;
+        uint32_t sector_size = 0;
+        bool ioctl_problem = false;
+        ret = 0;
+
+        /* Query the number of sectors on the disk */
+        ret = ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors);
+        if (ret != 0)
+            ioctl_problem = true;
+
+        /* Query the size of each sector */
+        ret = ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size);
+        if (ret != 0)
+            ioctl_problem = true;
+
+        /* If everything is ok */
+        if (ioctl_problem == false)
+            size = sectors * sector_size;
+
+        /* If a problem occurred with ioctl(), fallback to lseek() */
+        else
+            size = lseek(fd, 0LL, SEEK_END);
+    }
 #else
         size = lseek(fd, 0LL, SEEK_END);
         if (size < 0) {
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH v6] block/raw-posix.c: Fixes raw_getlength() on Mac OS X so that it reports the correct length of a real CD

[Peter Maydell]

On 13 January 2015 at 20:07, Programmingkid <programmingkidx@gmail.com> wrote:
> Allows QEMU on Mac OS X to use a real cdrom again.
>
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>
> ---
> Added fallback code - uses lseek() if ioctl() fails.
>
>  block/raw-posix.c |   25 ++++++++++++++++++++++++-
>  1 files changed, 24 insertions(+), 1 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index e51293a..5815707 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1312,7 +1312,30 @@ again:
>          if (size == 0)
>  #endif
>  #if defined(__APPLE__) && defined(__MACH__)
> -        size = LLONG_MAX;
> +    {
> +        uint64_t sectors = 0;
> +        uint32_t sector_size = 0;
> +        bool ioctl_problem = false;
> +        ret = 0;
> +
> +        /* Query the number of sectors on the disk */
> +        ret = ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors);
> +        if (ret != 0)
> +            ioctl_problem = true;
> +
> +        /* Query the size of each sector */
> +        ret = ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size);
> +        if (ret != 0)
> +            ioctl_problem = true;
> +
> +        /* If everything is ok */
> +        if (ioctl_problem == false)
> +            size = sectors * sector_size;
> +
> +        /* If a problem occurred with ioctl(), fallback to lseek() */
> +        else
> +            size = lseek(fd, 0LL, SEEK_END);
> +    }
>  #else

This fixes the "make check" problem, but you're not catching
the possibility of lseek failing. (Also the if() statements
are all missing braces our coding style requires.) You can
avoid that bool flag like this:

    {
        uint64_t sectors = 0;
        uint32_t sector_size = 0;

        if (ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors) == 0
            && ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) == 0) {
            size = sectors * sector_size;
        } else {
            size = lseek(fd, 0LL, SEEK_END);
            if (size < 0) {
                return -errno;
            }
        }
    }

thanks
-- PMM

Re: [Qemu-devel] [PATCH v6] block/raw-posix.c: Fixes raw_getlength() on Mac OS X so that it reports the correct length of a real CD

[Programmingkid]

On Jan 14, 2015, at 12:02 PM, Peter Maydell wrote:

> On 13 January 2015 at 20:07, Programmingkid <programmingkidx@gmail.com> wrote:
>> Allows QEMU on Mac OS X to use a real cdrom again.
>> 
>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>> 
>> ---
>> Added fallback code - uses lseek() if ioctl() fails.
>> 
>> block/raw-posix.c |   25 ++++++++++++++++++++++++-
>> 1 files changed, 24 insertions(+), 1 deletions(-)
>> 
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index e51293a..5815707 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -1312,7 +1312,30 @@ again:
>>         if (size == 0)
>> #endif
>> #if defined(__APPLE__) && defined(__MACH__)
>> -        size = LLONG_MAX;
>> +    {
>> +        uint64_t sectors = 0;
>> +        uint32_t sector_size = 0;
>> +        bool ioctl_problem = false;
>> +        ret = 0;
>> +
>> +        /* Query the number of sectors on the disk */
>> +        ret = ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors);
>> +        if (ret != 0)
>> +            ioctl_problem = true;
>> +
>> +        /* Query the size of each sector */
>> +        ret = ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size);
>> +        if (ret != 0)
>> +            ioctl_problem = true;
>> +
>> +        /* If everything is ok */
>> +        if (ioctl_problem == false)
>> +            size = sectors * sector_size;
>> +
>> +        /* If a problem occurred with ioctl(), fallback to lseek() */
>> +        else
>> +            size = lseek(fd, 0LL, SEEK_END);
>> +    }
>> #else
> 
> This fixes the "make check" problem, but you're not catching
> the possibility of lseek failing. (Also the if() statements
> are all missing braces our coding style requires.) You can
> avoid that bool flag like this:
> 
>    {
>        uint64_t sectors = 0;
>        uint32_t sector_size = 0;
> 
>        if (ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors) == 0
>            && ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) == 0) {
>            size = sectors * sector_size;
>        } else {
>            size = lseek(fd, 0LL, SEEK_END);
>            if (size < 0) {
>                return -errno;
>            }
>        }
>    }

Yeah, your code looks great. I will make a patch for it if you need me to. 

[Qemu-devel] [PATCH v7] block/raw-posix.c: Fixes raw_getlength() on Mac OS X so that it reports the correct length of a real CD

[Programmingkid]

This patch allows Mac OS X to use a real CDROM disc in QEMU. Testing this patch will require using QEMU v2.2.0 because the current git version has a bug in it that prevents /dev/cdrom from being used. "make check" did pass and my Debian boot disc did work. 

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

---
Totally rewritten.

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

diff --git a/block/raw-posix.c b/block/raw-posix.c
index e51293a..a95cfcf 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1312,7 +1312,20 @@ again:
         if (size == 0)
 #endif
 #if defined(__APPLE__) && defined(__MACH__)
-        size = LLONG_MAX;
+   {
+       uint64_t sectors = 0;
+       uint32_t sector_size = 0;
+
+       if (ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors) == 0
+           && ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) == 0) {
+           size = sectors * sector_size;
+       } else {
+           size = lseek(fd, 0LL, SEEK_END);
+           if (size < 0) {
+               return -errno;
+           }
+       }
+   }
 #else
         size = lseek(fd, 0LL, SEEK_END);
         if (size < 0) {
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH v7] block/raw-posix.c: Fixes raw_getlength() on Mac OS X so that it reports the correct length of a real CD

[Markus Armbruster]

Programmingkid <programmingkidx@gmail.com> writes:

> This patch allows Mac OS X to use a real CDROM disc in QEMU. Testing this patch will require using QEMU v2.2.0 because the current git version has a bug in it that prevents /dev/cdrom from being used. "make check" did pass and my Debian boot disc did work. 
>
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>

Subject line is too long, and the body lacks line breaks.  Suggest
something like:

    block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

    This patch allows Mac OS X to use a real CDROM disc in QEMU.
    Testing this patch will require using QEMU v2.2.0 because the
    current git version has a bug in it that prevents /dev/cdrom from
    being used.  "make check" did pass and my Debian boot disc did work.

Re: [Qemu-devel] [PATCH v7] block/raw-posix.c: Fixes raw_getlength() on Mac OS X so that it reports the correct length of a real CD

[Peter Maydell]

On 15 January 2015 at 23:40, Programmingkid <programmingkidx@gmail.com> wrote:
> This patch allows Mac OS X to use a real CDROM disc in QEMU. Testing this patch will require using QEMU v2.2.0 because the current git version has a bug in it that prevents /dev/cdrom from being used. "make check" did pass and my Debian boot disc did work.
>
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>
> ---
> Totally rewritten.
>
>  block/raw-posix.c |   15 ++++++++++++++-
>  1 files changed, 14 insertions(+), 1 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index e51293a..a95cfcf 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1312,7 +1312,20 @@ again:
>          if (size == 0)
>  #endif
>  #if defined(__APPLE__) && defined(__MACH__)
> -        size = LLONG_MAX;
> +   {
> +       uint64_t sectors = 0;
> +       uint32_t sector_size = 0;
> +
> +       if (ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors) == 0
> +           && ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) == 0) {
> +           size = sectors * sector_size;
> +       } else {
> +           size = lseek(fd, 0LL, SEEK_END);
> +           if (size < 0) {
> +               return -errno;
> +           }
> +       }
> +   }
>  #else

You have the indentation here wrong -- the first '{' should
be at the same indent as the 'size = ' line you've deleted;
and as Markus says the commit message could be neatened up.
Otherwise, code looks good and it passes make check, so
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH v7] block/raw-posix.c: Fixes raw_getlength() on Mac OS X so that it reports the correct length of a real CD

[Programmingkid]

On Jan 16, 2015, at 3:22 AM, Markus Armbruster wrote:

> Programmingkid <programmingkidx@gmail.com> writes:
> 
>> This patch allows Mac OS X to use a real CDROM disc in QEMU. Testing this patch will require using QEMU v2.2.0 because the current git version has a bug in it that prevents /dev/cdrom from being used. "make check" did pass and my Debian boot disc did work. 
>> 
>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> 
> Subject line is too long, and the body lacks line breaks.  Suggest
> something like:
> 
>    block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

Unfortunately I can't change the subject line after posting the first version of the patch. Others have to know the history of this patch. I will try to include the file name in the future. 

> 
>    This patch allows Mac OS X to use a real CDROM disc in QEMU.
>    Testing this patch will require using QEMU v2.2.0 because the
>    current git version has a bug in it that prevents /dev/cdrom from
>    being used.  "make check" did pass and my Debian boot disc did work.

Ok. I just don't know when to break it. I guess I will stick to pinky length. 

Re: [Qemu-devel] [PATCH v7] block/raw-posix.c: Fixes raw_getlength() on Mac OS X so that it reports the correct length of a real CD

[Eric Blake]

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

On 01/19/2015 09:32 AM, Programmingkid wrote:
> 
> On Jan 16, 2015, at 3:22 AM, Markus Armbruster wrote:
> 
>> Programmingkid <programmingkidx@gmail.com> writes:
>>
>>> This patch allows Mac OS X to use a real CDROM disc in QEMU. Testing this patch will require using QEMU v2.2.0 because the current git version has a bug in it that prevents /dev/cdrom from being used. "make check" did pass and my Debian boot disc did work. 
>>>
>>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>>
>> Subject line is too long, and the body lacks line breaks.  Suggest
>> something like:
>>
>>    block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD
> 
> Unfortunately I can't change the subject line after posting the first version of the patch. Others have to know the history of this patch. I will try to include the file name in the future. 

Yes, you can. It's a bit trickier, in that you'll probably want to
include a mailing list archive link in v8 and explicitly point out that
the subject has changed, as well as a followup to v7 that points to the
list archive of v8 to help people find the changed name.  But there is
nothing technically stopping you from renaming a patch to something more
appropriate.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v7] block/raw-posix.c: Fixes raw_getlength() on Mac OS X so that it reports the correct length of a real CD

[Markus Armbruster]

Programmingkid <programmingkidx@gmail.com> writes:

> On Jan 16, 2015, at 3:22 AM, Markus Armbruster wrote:
>
>> Programmingkid <programmingkidx@gmail.com> writes:
>> 
>>> This patch allows Mac OS X to use a real CDROM disc in
>>> QEMU. Testing this patch will require using QEMU v2.2.0 because the
>>> current git version has a bug in it that prevents /dev/cdrom from
>>> being used. "make check" did pass and my Debian boot disc did work.
>>> 
>>> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>> 
>> Subject line is too long, and the body lacks line breaks.  Suggest
>> something like:
>> 
>>    block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD
>
> Unfortunately I can't change the subject line after posting the first
> version of the patch. Others have to know the history of this patch. I
> will try to include the file name in the future.

Oh yes, you can!

If you care about patch history, do this:

    block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

    This patch allows Mac OS X to use a real CDROM disc in QEMU.
    Testing this patch will require using QEMU v2.2.0 because the
    current git version has a bug in it that prevents /dev/cdrom from
    being used.  "make check" did pass and my Debian boot disc did work.

    Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
    ---
    v7:
    * Totally rewritten.
    * Subject changed from "block/raw-posix.c: Fixes raw_getlength() on
      Mac OS X so that it reports the correct length of a real CD"
    v6:
    [more history, if you care...]

A good commit message is worth a little inconvenience.

>>    This patch allows Mac OS X to use a real CDROM disc in QEMU.
>>    Testing this patch will require using QEMU v2.2.0 because the
>>    current git version has a bug in it that prevents /dev/cdrom from
>>    being used.  "make check" did pass and my Debian boot disc did work.
>
> Ok. I just don't know when to break it. I guess I will stick to pinky length. 

I recommend to limit line length to 70 characters.