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

[Kevin Wolf <kwolf@redhat.com>]

Am 25.11.2015 um 05:18 hat Programmingkid geschrieben:
> 
> 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. 

I doubt that other kind of devices start in /dev/cdrom, but  on second
thought, as this isn't an actual filename, but just a magic string that
qemu converts to a real CD-ROM drive, your change makes sense.

Perhaps it should be split into its own patch, though, because it's a
logically independent change.

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

Then you need to make sure that we actually don't open the file and
instead run the error path. This patch doesn't achieve that.

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

include/qemu-common.h:

    /**
     * strstart:
     * @str: string to test
     * @val: prefix string to look for
     * @ptr: NULL, or pointer to be written to indicate start of
     *       the remainder of the string
     *
     * Test whether @str starts with the prefix @val.
     * If it does (including the degenerate case where @str and @val
     * are equal) then return true. If @ptr is not NULL then a
     * pointer to the first character following the prefix is written
     * to it. If @val is not a prefix of @str then return false (and
     * @ptr is not written to).
     *
     * Returns: true if @str starts with prefix @val, false otherwise.
     */
    int strstart(const char *str, const char *val, const char **ptr);

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

Please write your code so that it makes sense today.

raw_open_common() won't ever return positive error numbers, that would
be insane and against all conventions. It's even less likely than making
0 an error return value (i.e. converting the function to return a
boolean value) - but I can't see either happening. We have reviews to
avoid such misguided changes.

The only thing that might theoretically happen is that positive values
could at some point denote different successful results. But if anyone
were to propose an insane change like that, it would be their job to
adapt the callers to the new interface.

Such a highly hypothetical change is not a reason to clutter our code
now.

Kevin