[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] ping [PATCH v14] block/raw-posix.c: Make physical devic
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host |
Date: |
Wed, 2 Mar 2016 10:02:31 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
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