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