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

[Peter Maydell]

On 28 December 2014 at 17:37, Programmingkid <address@hidden> wrote:
> On Dec 28, 2014, at 5:23 AM, Peter Maydell wrote:
>> On 28 December 2014 at 03:00, Programmingkid <address@hidden> wrote:
>>> #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.

QEMU general style is to assume that -1 is well known
as an error return from POSIX functions -- we check
for and return raw "-1" everywhere. (This is also pretty
widespread practice for most unix C programs I think.)

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

NB that scripts/checkpatch.pl can find most of this kind of
style nit (though it is not always right).

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

Reporting the error is the responsibility of the caller.
(For instance, if the user triggered this operation via
the QEMU monitor then we need to send the error message
there rather than standard output.) It is true that only
returning the errno is potentially throwing away a little
information about the cause of the error, but is it
really much more helpful for the user to know whether it's
the sector count or the sector size that couldn't be
retrieved, compared to a generic message about not being
able to determine the size of the block device? Upper levels
of the program are usually better placed to provide an
error message that relates to what the user was trying to do,
which is why low level functions defer to them.

(We do have a mechanism for passing more error information
than just an errno, which is the Error **errp idiom. In
theory we could change the API of this function to take an
errp, but it's not clear it's worth the effort, and in
any case that would be a separate patch.)

thanks
-- PMM