[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] ping [PATCH v14] block/raw-posix.c: Make p
From: |
Jeff Cody |
Subject: |
Re: [Qemu-devel] [Qemu-block] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host |
Date: |
Tue, 1 Mar 2016 11:25:35 -0500 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Tue, Mar 01, 2016 at 04:16:39PM +0100, 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.
>
> 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?
>
> Kevin
>
> > + }
> > +#endif /* defined(__APPLE__) && defined(__MACH__) */
> > }
> >
> > /* Since this does ioctl the device must be already opened */
> > --
> > 1.7.5.4
> >
> >
>
In addition to the above, the patch is still using quoted-printable
encoding. With Mutt at least, this makes applying patches cumbersome.
Whether or not this is an issue for Kevin's workflow, I don't know.
But if you are re-spinning a new patch, I recommend taking care of
that (git send-email will do the right thing).
Jeff