[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] block/raw-posix.c: Fixes raw_getlength() on
From: |
Programmingkid |
Subject: |
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 |
Date: |
Fri, 2 Jan 2015 11:47:46 -0500 |
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 <address@hidden>
>
> 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.