Re: [Qemu-block] [PATCH v6] raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Programmingkid]

On Nov 24, 2015, at 9:38 AM, Kevin Wolf wrote:
> 
> 
>> +    /* If using a physical device */
>> +    if (strstart(filename, "/dev/", NULL)) {
>> +        char bsdPath[MAXPATHLEN];
>> +
>> +        /* If the physical device is a cdrom */
>> +        if (strcmp(filename, "/dev/cdrom") == 0) {
> 
> The original code considered everything that starts in "/dev/cdrom", but
> this one only considers exact matches. Intentional?

Yes. CDROM's are handled differently from other kinds of devices. 

> 
> The outer strstart() check is redundant in this code as it is written.
> I'm not sure what you really wanted to do for the case that starts in
> "/dev/" but is different from "/dev/cdrom", but with this implementation
> nothing happens.
> 
>> +            io_iterator_t mediaIterator;
>> +            FindEjectableCDMedia(&mediaIterator);
>> +            GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath), flags);
>> +            if (bsdPath[0] == '\0') {
>> +                printf("Error: failed to obtain bsd path for optical 
>> drive!\n");
> 
> If this is really an error, shouldn't we actually set errp and return
> from the function? And if it's not an error, being silent sounds right.

It is an error. But there is a chance that unmounting the device's volume from
the desktop might fix the problem, so letting the function continue to the 
helpful
error messages would be beneficial to the user. 

>> 
>> +#if defined(__APPLE__) && defined(__MACH__)
>> +    /* if a physical device experienced an error while being opened */
>> +    if (strncmp(filename, "/dev/", 5) == 0 && (cdromOK == false || ret != 
>> 0)) {
> 
> Using strstart() would probably be more consistent.

I would really prefer to stick with strncmp(). It is ANSI C and very well 
documented.
A search for strstart() did not turn up any documentation on it. 

> 
> I asked in v5 whether ret > 0 was possible (because otherwise the two
> 'if (ret < 0)' blocks could be merged) and you said it was. Now I
> reviewed raw_open_common() and I must say that I can't see how this
> function would ever return anything other than 0 or a negative errno.

It is possible the raw_open_common() function could be changed in the future
to produce positive error numbers. I think checking for anything that isn't zero
is the best thing to do.