[Qemu-devel] [PATCH v3] 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 v3] 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 fixes the problem with raw_getlength() on Mac OS X so that it actually calculates the correct size of a volume. It has been updated to fix certain coding style issues. Booting and using a real CD in QEMU works again. 

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

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

diff --git a/block/raw-posix.c b/block/raw-posix.c
index e51293a..a090c9c 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1312,7 +1312,24 @@ again:
         if (size == 0)
 #endif
 #if defined(__APPLE__) && defined(__MACH__)
-        size = LLONG_MAX;
+   {
+        uint64_t sectors = 0;
+        uint32_t sectorSize = 0;
+        int ret;
+
+        /* Query the number of sectors on the disk */
+        ret = ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors);
+        if (ret == -1) {
+           return -errno;
+        }
+
+        /* Query the size of each sector */
+        ret = ioctl(fd, DKIOCGETBLOCKSIZE, &sectorSize);
+        if (ret == -1) {
+           return -errno;
+        }
+        size = sectors * sectorSize;
+   }
 #else
         size = lseek(fd, 0LL, SEEK_END);
         if (size < 0) {
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH v3] 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 28 December 2014 at 21:18, Programmingkid <programmingkidx@gmail.com> wrote:
> This patch fixes the problem with raw_getlength() on Mac OS X so that it actually calculates the correct size of a volume. It has been updated to fix certain coding style issues. Booting and using a real CD in QEMU works again.
>
> signed-off-by: John Arbuckle <programmingkidx@gmail.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

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

[Stefan Hajnoczi]

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

On Sun, Dec 28, 2014 at 04:18:38PM -0500, Programmingkid wrote:

Suggestion for concise subject line:

  block/raw-posix: fix raw_getlength() for host CD-ROMs on Mac

> This patch fixes the problem with raw_getlength() on Mac OS X so that it actually calculates the correct size of a volume. It has been updated to fix certain coding style issues. Booting and using a real CD in QEMU works again. 

Plain text emails are usually wrapped at 72 characters.  It makes it
easier to read the git log if you wrap lines.

Good job braving the nasty nest of #ifdefs in raw_getlength() :).  The
code change looks good.  Minor changes below - normally I'd make them
while merging your patch but I don't compile QEMU on Mac so I can't
compile test it.  Please send a new version of this patch.

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

Not sure if tags are case-sensistive but it is usually written as
"Signed-off-by:".  The git -s option does this for you, that's usually
more convenient than typing it out manually.

> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index e51293a..a090c9c 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1312,7 +1312,24 @@ again:
>          if (size == 0)
>  #endif
>  #if defined(__APPLE__) && defined(__MACH__)
> -        size = LLONG_MAX;
> +   {
> +        uint64_t sectors = 0;
> +        uint32_t sectorSize = 0;

Please follow QEMU coding style and use lower_case_with_underscore
variable names.

> +        int ret;

No need to shadow the local variable that was already declared at the
top of the function.  Please drop this.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v3] 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 2, 2015, at 9:38 AM, Stefan Hajnoczi wrote:

> On Sun, Dec 28, 2014 at 04:18:38PM -0500, Programmingkid wrote:
> 
> Suggestion for concise subject line:
> 
>  block/raw-posix: fix raw_getlength() for host CD-ROMs on Mac
> 
>> This patch fixes the problem with raw_getlength() on Mac OS X so that it actually calculates the correct size of a volume. It has been updated to fix certain coding style issues. Booting and using a real CD in QEMU works again. 
> 
> Plain text emails are usually wrapped at 72 characters.  It makes it
> easier to read the git log if you wrap lines.

On my email program, the lines just wrap at the width of the window they are in. 
Maybe there is a feature in your email program to wrap lines.

> 
> Good job braving the nasty nest of #ifdefs in raw_getlength() :).  The
> code change looks good.  Minor changes below - normally I'd make them
> while merging your patch but I don't compile QEMU on Mac so I can't
> compile test it.  Please send a new version of this patch.

Thank you for the kind remarks. I can't believe some of the code in that file was 
actually committed. I think someone should remove most of the #ifdefs and 
pretty up the file. I would probably do something like mac_raw_getlength(), 
windows_raw_getlength(), linux_raw_getlength(), sun_raw_getlength()...
Then just use the correct function for the host. 

> 
>> 
>> signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> 
> Not sure if tags are case-sensistive but it is usually written as
> "Signed-off-by:".  The git -s option does this for you, that's usually
> more convenient than typing it out manually.

Ok, that will be easy to fix.

> 
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index e51293a..a090c9c 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -1312,7 +1312,24 @@ again:
>>         if (size == 0)
>> #endif
>> #if defined(__APPLE__) && defined(__MACH__)
>> -        size = LLONG_MAX;
>> +   {
>> +        uint64_t sectors = 0;
>> +        uint32_t sectorSize = 0;
> 
> Please follow QEMU coding style and use lower_case_with_underscore
> variable names.

Fixed.

> 
>> +        int ret;
> 
> No need to shadow the local variable that was already declared at the
> top of the function.  Please drop this.

Done.

Thank you for reviewing my patch.

Re: [Qemu-devel] [PATCH v3] 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 2 January 2015 at 16:47, Programmingkid <programmingkidx@gmail.com> wrote:
> On Jan 2, 2015, at 9:38 AM, Stefan Hajnoczi wrote:
>> Plain text emails are usually wrapped at 72 characters.  It makes it
>> easier to read the git log if you wrap lines.
>
> On my email program, the lines just wrap at the width of the window they are in.
> Maybe there is a feature in your email program to wrap lines.

The standard git tools for applying patches from email don't
wrap lines -- they assume that the sender has inserted manual
carriage returns at the right places (and that if there are
long lines it's because they should be long, for instance to
avoid quoted command lines being wrapped). This is just one of
those things where you need to follow the conventions to make
life easier for the recipients, I'm afraid.

>> Good job braving the nasty nest of #ifdefs in raw_getlength() :).  The
>> code change looks good.  Minor changes below - normally I'd make them
>> while merging your patch but I don't compile QEMU on Mac so I can't
>> compile test it.  Please send a new version of this patch.
>
> Thank you for the kind remarks. I can't believe some of the code in that file was
> actually committed. I think someone should remove most of the #ifdefs and
> pretty up the file. I would probably do something like mac_raw_getlength(),
> windows_raw_getlength(), linux_raw_getlength(), sun_raw_getlength()...
> Then just use the correct function for the host.

Yes; you can see already that half of the BSDs have been abstracted
out. The difficulty is that to a first approximation none of the
regular developers here uses anything except Linux, and it's always
a bit tricky to make changes which you aren't testing...

thanks
-- PMM