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 <programmingkidx@gmail.com>]

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.