[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devic
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host |
Date: |
Wed, 2 Mar 2016 17:49:11 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 02.03.2016 um 17:39 hat Programmingkid geschrieben:
>
> 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 <address@hidden>
> >>>>
> >>>> ---
> >>>> 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;
> }
Looks right to me.
Kevin