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

[Programmingkid]

On Dec 28, 2014, at 5:23 AM, Peter Maydell wrote:

> On 28 December 2014 at 03:00, Programmingkid <address@hidden> wrote:
>> Here is version 2 of the patch. All the suggestions have been implemented.
> 
> Thanks.
> 
> Last round of nits, but the rest of the change is fine.
> If you post v3 as its own email that will make it easier
> to apply (most of our patch-handling tools assume patches
> come in their own emails, rather than embedded in other
> threads).

Ok. Not a problem.

>> signed-off-by: John Arbuckle <address@hidden>
>> 
>> ---
>> block/raw-posix.c |   20 +++++++++++++++++++-
>> 1 files changed, 19 insertions(+), 1 deletions(-)
>> 
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index e51293a..0148161 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -1312,7 +1312,25 @@ again:
>>         if (size == 0)
>> #endif
>> #if defined(__APPLE__) && defined(__MACH__)
>> -        size = LLONG_MAX;
>> +        #define IOCTL_ERROR_VALUE -1
> 
> You don't need this -- just compare against -1.

The value -1 does communicate the fact that I am looking for an error value. 
The code is more self documenting with the constant.

> 
> Open brace here.
> 
>> +        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 == IOCTL_ERROR_VALUE) {
> 
> Spaces needed between "if" and "(".

Ok, it would look nicer with the space.

> 
>> +           printf("\n\nWarning: problem detected retrieving sector 
>> count!\n\n");
> 
> Delete these printfs.

If we did that, how would the user know something went wrong? If something did 
go wrong, the user would see these messages and be able to trace the problem 
directly to the source.

> 
>> +           return -errno;
>> +        }
>> +
>> +        /* Query the size of each sector */
>> +        ret = ioctl(fd, DKIOCGETBLOCKSIZE, &sectorSize);
>> +        if(ret == IOCTL_ERROR_VALUE) {
>> +           printf("\n\nWarning: problem detected retrieving sector 
>> size!\n\n");
>> +           return -errno;
>> +        }
>> +        size = sectors * sectorSize;
> 
> Close brace here.
> 
>> #else
>>         size = lseek(fd, 0LL, SEEK_END);
>>         if (size < 0) {
>> --
>> 1.7.5.4
> 
> thanks
> -- PMM