From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57984) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ab9xf-0008Rf-Hs for qemu-devel@nongnu.org; Wed, 02 Mar 2016 11:49:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ab9xe-0006Ik-F4 for qemu-devel@nongnu.org; Wed, 02 Mar 2016 11:49:19 -0500 Date: Wed, 2 Mar 2016 17:49:11 +0100 From: Kevin Wolf Message-ID: <20160302164911.GF4494@noname.redhat.com> References: <3BAC5050-04A8-44DA-B65F-424CFE5615DA@gmail.com> <20160301151639.GF4250@noname.str.redhat.com> <20160302090231.GB4494@noname.redhat.com> <1F537F4B-2881-46B7-81B2-D36CDDA7EA21@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1F537F4B-2881-46B7-81B2-D36CDDA7EA21@gmail.com> Subject: Re: [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Programmingkid Cc: qemu-devel qemu-devel , Qemu-block 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 > >>>> > >>>> --- > >>>> 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