[Qemu-devel] [PATCH v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Qemu-devel] [PATCH v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Programmingkid]

Subject was: 
Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() 
on Mac OS X so that it reports the correct length of a real 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>

---
Fixed code indentation to be inline with removed
size = LLONG_MAX.

 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..fa431b2 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 v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Markus Armbruster]

Programmingkid <programmingkidx@gmail.com> writes:

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

Patch history information goes...

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

... below the --- divider.

> Fixed code indentation to be inline with removed
> size = LLONG_MAX.
>
>  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..fa431b2 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;

Ignorant question: why do you need to initialize these?

Patch looks plausible otherwise, but know nothing about Macs :)

> +
> +            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) {

Re: [Qemu-devel] [PATCH v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Programmingkid]

On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote:

> Programmingkid <programmingkidx@gmail.com> writes:
> 
>> Subject was: 
>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() 
>> on Mac OS X so that it reports the correct length of a real CD
> 
> Patch history information goes...
> 
>> 
>> 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>
>> 
>> ---
> 
> ... below the --- divider.

I thought I did this. The information above is the description of the patch. 
Not its history.

> 
>> Fixed code indentation to be inline with removed
>> size = LLONG_MAX.
>> 
>> 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..fa431b2 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;
> 
> Ignorant question: why do you need to initialize these?

We don't. It's just a habit. 

> 
> Patch looks plausible otherwise, but know nothing about Macs :)

It does do the job.

> 
>> +
>> +            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) {

Re: [Qemu-devel] [PATCH v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Eric Blake]

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

On 01/20/2015 07:29 AM, Programmingkid wrote:
> 
> On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote:
> 
>> Programmingkid <programmingkidx@gmail.com> writes:
>>
>>> Subject was: 
>>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() 
>>> on Mac OS X so that it reports the correct length of a real CD
>>
>> Patch history information goes...

>>
>> ... below the --- divider.
> 
> I thought I did this. The information above is the description of the patch. 
> Not its history.

Anything that mentions 'v7' is history.  When you read 'git log', you
will not see mentions of 'v7', because no one cares how many tries it
took to get a patch into git.  Knowing about v7 only matters to the
reviewers of v8, hence it is patch history that belongs after the divider.


>>> +
>>> +            if (ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors) == 0
>>> +               && ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) == 0) {

Indentation looks off here.

-- 
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 v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Programmingkid]

On Jan 20, 2015, at 10:22 AM, Eric Blake wrote:

> On 01/20/2015 07:29 AM, Programmingkid wrote:
>> 
>> On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote:
>> 
>>> Programmingkid <programmingkidx@gmail.com> writes:
>>> 
>>>> Subject was: 
>>>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() 
>>>> on Mac OS X so that it reports the correct length of a real CD
>>> 
>>> Patch history information goes...
> 
>>> 
>>> ... below the --- divider.
>> 
>> I thought I did this. The information above is the description of the patch. 
>> Not its history.
> 
> Anything that mentions 'v7' is history.  When you read 'git log', you
> will not see mentions of 'v7', because no one cares how many tries it
> took to get a patch into git.  Knowing about v7 only matters to the
> reviewers of v8, hence it is patch history that belongs after the divider.

Ok. 

> 
> 
>>>> +
>>>> +            if (ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors) == 0
>>>> +               && ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) == 0) {
> 
> Indentation looks off here.

It does look a little odd, but it also communicates that this is one statement (IMHO). 

Re: [Qemu-devel] [PATCH v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Markus Armbruster]

Programmingkid <programmingkidx@gmail.com> writes:

> On Jan 20, 2015, at 10:22 AM, Eric Blake wrote:
>
>> On 01/20/2015 07:29 AM, Programmingkid wrote:
>>> 
>>> On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote:
>>> 
>>>> Programmingkid <programmingkidx@gmail.com> writes:
>>>> 
>>>>> Subject was: 
>>>>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() 
>>>>> on Mac OS X so that it reports the correct length of a real CD
>>>> 
>>>> Patch history information goes...
>> 
>>>> 
>>>> ... below the --- divider.
>>> 
>>> I thought I did this. The information above is the description of the patch. 
>>> Not its history.
>> 
>> Anything that mentions 'v7' is history.  When you read 'git log', you
>> will not see mentions of 'v7', because no one cares how many tries it
>> took to get a patch into git.  Knowing about v7 only matters to the
>> reviewers of v8, hence it is patch history that belongs after the divider.
>
> Ok. 
>
>> 
>> 
>>>>> +
>>>>> +            if (ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors) == 0
>>>>> +               && ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) == 0) {
>> 
>> Indentation looks off here.
>
> It does look a little odd, but it also communicates that this is one
> statement (IMHO).

It's not how the rest of QEMU is indented.  Please try to blend in :)

I feel bad about notpicking v8 of an obviously useful and patch that is
basically just fine except for these little things.  Thanks for
persevering!

Re: [Qemu-devel] [PATCH v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Programmingkid]

On Jan 20, 2015, at 3:28 PM, Markus Armbruster wrote:

> Programmingkid <programmingkidx@gmail.com> writes:
> 
>> On Jan 20, 2015, at 10:22 AM, Eric Blake wrote:
>> 
>>> On 01/20/2015 07:29 AM, Programmingkid wrote:
>>>> 
>>>> On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote:
>>>> 
>>>>> Programmingkid <programmingkidx@gmail.com> writes:
>>>>> 
>>>>>> Subject was: 
>>>>>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() 
>>>>>> on Mac OS X so that it reports the correct length of a real CD
>>>>> 
>>>>> Patch history information goes...
>>> 
>>>>> 
>>>>> ... below the --- divider.
>>>> 
>>>> I thought I did this. The information above is the description of the patch. 
>>>> Not its history.
>>> 
>>> Anything that mentions 'v7' is history.  When you read 'git log', you
>>> will not see mentions of 'v7', because no one cares how many tries it
>>> took to get a patch into git.  Knowing about v7 only matters to the
>>> reviewers of v8, hence it is patch history that belongs after the divider.
>> 
>> Ok. 
>> 
>>> 
>>> 
>>>>>> +
>>>>>> +            if (ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors) == 0
>>>>>> +               && ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) == 0) {
>>> 
>>> Indentation looks off here.
>> 
>> It does look a little odd, but it also communicates that this is one
>> statement (IMHO).
> 
> It's not how the rest of QEMU is indented.  Please try to blend in :)
> 
> I feel bad about notpicking v8 of an obviously useful and patch that is
> basically just fine except for these little things.  Thanks for
> persevering!

Thanks for your encouragement. I personally would have had that whole if
condition on one line, but others want an 80 line maximum. 

Re: [Qemu-devel] [PATCH v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Markus Armbruster]

Programmingkid <programmingkidx@gmail.com> writes:

> On Jan 20, 2015, at 3:28 PM, Markus Armbruster wrote:
>
>> Programmingkid <programmingkidx@gmail.com> writes:
>> 
>>> On Jan 20, 2015, at 10:22 AM, Eric Blake wrote:
>>> 
>>>> On 01/20/2015 07:29 AM, Programmingkid wrote:
>>>>> 
>>>>> On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote:
>>>>> 
>>>>>> Programmingkid <programmingkidx@gmail.com> writes:
>>>>>> 
>>>>>>> Subject was: 
>>>>>>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() 
>>>>>>> on Mac OS X so that it reports the correct length of a real CD
>>>>>> 
>>>>>> Patch history information goes...
>>>> 
>>>>>> 
>>>>>> ... below the --- divider.
>>>>> 
>>>>> I thought I did this. The information above is the description of
>>>>> the patch.
>>>>> Not its history.
>>>> 
>>>> Anything that mentions 'v7' is history.  When you read 'git log', you
>>>> will not see mentions of 'v7', because no one cares how many tries it
>>>> took to get a patch into git.  Knowing about v7 only matters to the
>>>> reviewers of v8, hence it is patch history that belongs after the divider.
>>> 
>>> Ok. 
>>> 
>>>> 
>>>> 
>>>>>>> +
>>>>>>> +            if (ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors) == 0
>>>>>>> +               && ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) == 0) {
>>>> 
>>>> Indentation looks off here.
>>> 
>>> It does look a little odd, but it also communicates that this is one
>>> statement (IMHO).
>> 
>> It's not how the rest of QEMU is indented.  Please try to blend in :)
>> 
>> I feel bad about notpicking v8 of an obviously useful and patch that is
>> basically just fine except for these little things.  Thanks for
>> persevering!
>
> Thanks for your encouragement. I personally would have had that whole if
> condition on one line, but others want an 80 line maximum. 

Humans tend to have trouble following long lines with their eyes (I sure
do).  Typographic manuals suggest to limit columns to roughly 60
characters for exactly that reason[*].  Code can be a bit wider since
it's commonly indented[**].

For commit messages, 70 characters is already a compromise between
legibility and "writability".


[*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style

[**] Deep nesting is no excuse for exceeding the width limit, because
deep nesting is no excuse for *anything* :)

Re: [Qemu-devel] [PATCH v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Programmingkid]

On Jan 21, 2015, at 2:54 AM, Markus Armbruster wrote:

> Programmingkid <programmingkidx@gmail.com> writes:
> 
>> On Jan 20, 2015, at 3:28 PM, Markus Armbruster wrote:
>> 
>>> Programmingkid <programmingkidx@gmail.com> writes:
>>> 
>>>> On Jan 20, 2015, at 10:22 AM, Eric Blake wrote:
>>>> 
>>>>> On 01/20/2015 07:29 AM, Programmingkid wrote:
>>>>>> 
>>>>>> On Jan 20, 2015, at 3:33 AM, Markus Armbruster wrote:
>>>>>> 
>>>>>>> Programmingkid <programmingkidx@gmail.com> writes:
>>>>>>> 
>>>>>>>> Subject was: 
>>>>>>>> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() 
>>>>>>>> on Mac OS X so that it reports the correct length of a real CD
>>>>>>> 
>>>>>>> Patch history information goes...
>>>>> 
>>>>>>> 
>>>>>>> ... below the --- divider.
>>>>>> 
>>>>>> I thought I did this. The information above is the description of
>>>>>> the patch.
>>>>>> Not its history.
>>>>> 
>>>>> Anything that mentions 'v7' is history.  When you read 'git log', you
>>>>> will not see mentions of 'v7', because no one cares how many tries it
>>>>> took to get a patch into git.  Knowing about v7 only matters to the
>>>>> reviewers of v8, hence it is patch history that belongs after the divider.
>>>> 
>>>> Ok. 
>>>> 
>>>>> 
>>>>> 
>>>>>>>> +
>>>>>>>> +            if (ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors) == 0
>>>>>>>> +               && ioctl(fd, DKIOCGETBLOCKSIZE, &sector_size) == 0) {
>>>>> 
>>>>> Indentation looks off here.
>>>> 
>>>> It does look a little odd, but it also communicates that this is one
>>>> statement (IMHO).
>>> 
>>> It's not how the rest of QEMU is indented.  Please try to blend in :)
>>> 
>>> I feel bad about notpicking v8 of an obviously useful and patch that is
>>> basically just fine except for these little things.  Thanks for
>>> persevering!
>> 
>> Thanks for your encouragement. I personally would have had that whole if
>> condition on one line, but others want an 80 line maximum. 
> 
> Humans tend to have trouble following long lines with their eyes (I sure
> do).  Typographic manuals suggest to limit columns to roughly 60
> characters for exactly that reason[*].  Code can be a bit wider since
> it's commonly indented[**].
> 
> For commit messages, 70 characters is already a compromise between
> legibility and "writability".
> 
> 
> [*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style
> 
> [**] Deep nesting is no excuse for exceeding the width limit, because
> deep nesting is no excuse for *anything* :)

<Puts up white flag> I will indent my code at around the 60 to 70
character size.

Re: [Qemu-devel] [PATCH v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Stefan Hajnoczi]

On Mon, Jan 19, 2015 at 10:12 PM, Programmingkid
<programmingkidx@gmail.com> wrote:
> Subject was:
> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength()
> on Mac OS X so that it reports the correct length of a real 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>
>
> ---
> Fixed code indentation to be inline with removed
> size = LLONG_MAX.
>
>  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..fa431b2 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) {

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Peter Maydell]

On 20 January 2015 at 14:46, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Tested-by: Peter Maydell <peter.maydell@linaro.org>
(at least as far as OSX compile and make check goes; my Mac
doesn't have a cdrom drive).

-- PMM

Re: [Qemu-devel] [PATCH v8] block/raw-posix.c: Fix raw_getlength() on Mac OS X for CD

[Kevin Wolf]

Am 19.01.2015 um 23:12 hat Programmingkid geschrieben:
> Subject was: 
> Re: [PATCH v7] block/raw-posix.c: Fixes raw_getlength() 
> on Mac OS X so that it reports the correct length of a real 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>

Thanks, applied to the block branch.

Kevin