On Mar 2, 2016, at 4:02 AM, Kevin Wolf wrote: > Am 02.03.2016 um 04:32 hat Programmingkid geschrieben: >> >> On Mar 1, 2016, at 10:16 AM, Kevin Wolf wrote: >> >>> Am 29.02.2016 um 16:17 hat Programmingkid geschrieben: >>>> I do think this patch is ready to be added to QEMU. I have listened to >>>> what you said and implemented your changes. >>>> >>>> https://patchwork.ozlabs.org/patch/579325/ >>>> >>>> Mac OS X can be picky when it comes to allowing the user >>>> to use physical devices in QEMU. Most mounted volumes >>>> appear to be off limits to QEMU. If an issue is detected, >>>> a message is displayed showing the user how to unmount a >>>> volume. Now QEMU uses both CD and DVD media. >>>> >>>> Signed-off-by: John Arbuckle <programmingk...@gmail.com> >>>> >>>> --- >>>> Changed filename variable to const char * type. >>>> Removed snprintf call for filename variable. >>>> filename is set to bsd_path if using a physical device that isn't a DVD or >>>> CD. >>> >>>> @@ -2112,33 +2166,57 @@ static int hdev_open(BlockDriverState *bs, QDict >>>> *options, int flags, >>>> >>>> #if defined(__APPLE__) && defined(__MACH__) >>>> const char *filename = qdict_get_str(options, "filename"); >>>> + char bsd_path[MAXPATHLEN] = ""; >>>> + bool error_occurred = false; >>>> + >>> >>> This line adds trailing whitespace. >>> >>>> @@ -2147,7 +2225,16 @@ static int hdev_open(BlockDriverState *bs, QDict >>>> *options, int flags, >>>> if (local_err) { >>>> error_propagate(errp, local_err); >>>> } >>>> - return ret; >>>> +#if defined(__APPLE__) && defined(__MACH__) >>>> + if (*bsd_path) { >>>> + filename = bsd_path; >>>> + } >>>> + /* if a physical device experienced an error while being opened */ >>>> + if (strncmp(filename, "/dev/", 5) == 0) { >>>> + print_unmounting_directions(filename); >>>> + return -1; >>> >>> Please use a negative errno number instead of -1. >> >> Is this ok: >> return -EPERM; >> >> According to http://man7.org/linux/man-pages/man3/errno.3.html, it means >> "operation not permitted". > > Well, to be honest I don't understand why there is a different error > code here to begin with. Maybe when you add the "return ret" back after > the #endif you can just leave out this line and return the real error > code this way.
I like this idea more. > > If for some reason (that I fail to understand) ret doesn't contain an > appropriate error code in this case, though, you can return a constant. > If it's related to permissions, -EPERM is okay, otherwise it's probably > not. I don't see a connection between paths starting with /dev/ and > there being a permission problem. I have placed "return ret;" right after the #endif and removed the "return -1" in the if condition because it is now redundant. Does this look right: if (ret < 0) { if (local_err) { error_propagate(errp, local_err); } #if defined(__APPLE__) && defined(__MACH__) if (*bsd_path) { filename = bsd_path; } /* if a physical device experienced an error while being opened */ if (strncmp(filename, "/dev/", 5) == 0) { print_unmounting_directions(filename); } #endif /* defined(__APPLE__) && defined(__MACH__) */ return ret; } > >> Did you want this -1 changed also? >> +hdev_open_Mac_error: >> + g_free(mediaType); >> + if (mediaIterator) { >> + IOObjectRelease(mediaIterator); >> + } >> + if (error_occurred) { >> + return -1; >> + } > > Yes, please, I missed that one. We don't have a valid ret here, so maybe > -ENOENT would be the closest one? Ok. > >>> But more importantly: What happened with the return that you removed >>> above? Even in the non-Apple case, we don't return an error now, but >>> continue in this function. This looks certainly wrong. Did you intend to >>> move it to below the #ifdef block? >> >> Good catch. I will place it right below the #endif /* defined(__APPLE__) && >> defined(__MACH__) */. > > Thanks. > > Kevin Thank you.