[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v6] raw-posix.c: Make physical devices usable in
From: |
Programmingkid |
Subject: |
Re: [Qemu-block] [PATCH v6] raw-posix.c: Make physical devices usable in QEMU under Mac OS X host |
Date: |
Tue, 24 Nov 2015 23:18:50 -0500 |
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.