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

On Dec 27, 2014, at 8:19 PM, Peter Maydell wrote:

> On 28 December 2014 at 00:36, Programmingkid <programmingkidx@gmail.com> wrote:
>> The raw_getlength() function under Mac OS X incorrectly returned a constant value instead of calculating the size of a real CD-ROM disc. This patch fixes this problem and makes booting from a real CD-ROM disc possible again under Mac OS X.
>> 
>> signed-off-by: John Arbuckle <programmingkidx@gmail.com>
> 
> Thanks. (My Mac doesn't have a cdrom...)
> 
>> ---
>> block/raw-posix.c |   11 ++++++++++-
>> 1 files changed, 10 insertions(+), 1 deletions(-)
>> 
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index e51293a..d723133 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -1312,7 +1312,16 @@ again:
>>         if (size == 0)
>> #endif
>> #if defined(__APPLE__) && defined(__MACH__)
>> -        size = LLONG_MAX;
>> +        // Query the number of sectors on the disk
> 
> I note in passing that this whole function is a total mess
> -- look at that "if (size == 0)" in the previous #ifdef...
> However since I think it's impossible to both have the
> previous #ifdef and this one enabled it won't cause an issue.

I agree. This file needs a lot of work. 

> 
>> +        uint64_t sectors = 0;
>> +        ioctl(fd, DKIOCGETBLOCKCOUNT, &sectors);
> 
> You need to check the error return from these ioctl calls.
> 
>> +
>> +        // Query the size of each sector
>> +        uint32_t sectorSize = 0;
>> +        ioctl(fd, DKIOCGETBLOCKSIZE, &sectorSize);
>> +
>> +        size = sectors * sectorSize;
>> +        //printf("size of disc = %d MB\n", size/(1024*1024));
>> #else
>>         size = lseek(fd, 0LL, SEEK_END);
>>         if (size < 0) {
> 
> Minor style issues you might want to fix for v2:
> * use /* */ comments, not //
> * declare variables only at the top of {} code blocks
> * don't leave commented out debug printfs in code
> 
> thanks
> -- PMM

Here is version 2 of the patch. All the suggestions have been implemented.

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

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

On 28 December 2014 at 03:00, Programmingkid <programmingkidx@gmail.com> 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).

> signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>
> ---
>  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.

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

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

Delete these printfs.

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

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

> On 28 December 2014 at 03:00, Programmingkid <programmingkidx@gmail.com> 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 <programmingkidx@gmail.com>
>> 
>> ---
>> 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

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