From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47160) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQenT-0001bu-P4 for qemu-devel@nongnu.org; Tue, 02 Feb 2016 12:31:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQenS-0006s2-Ks for qemu-devel@nongnu.org; Tue, 02 Feb 2016 12:31:23 -0500 Date: Tue, 2 Feb 2016 17:31:11 +0000 From: "Daniel P. Berrange" Message-ID: <20160202173111.GN18461@redhat.com> References: <1CF37786-9AAA-4E5E-B571-DE20E7A463EB@gmail.com> <20160202171606.GM18461@redhat.com> <72A30A67-5846-4835-B8D2-B7F545F76C0B@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <72A30A67-5846-4835-B8D2-B7F545F76C0B@gmail.com> Subject: Re: [Qemu-devel] ping: [PATCH v13] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Programmingkid Cc: Kevin Wolf , qemu-devel qemu-devel , Qemu-block On Tue, Feb 02, 2016 at 12:28:24PM -0500, Programmingkid wrote: > > On Feb 2, 2016, at 12:16 PM, Daniel P. Berrange wrote: > > > On Tue, Feb 02, 2016 at 12:08:31PM -0500, Programmingkid wrote: > >> https://patchwork.ozlabs.org/patch/570128/ > >> > >> 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 a character array. > >> Changed how filename was set to bsd_path's value by using snprintf(). > > > > Whats the rationale here ? Using pre-allocated fixed > > length arrays is pretty bad practice in general, but > > especially so for filenames > > With an automatic variable there is no worry about when to release it. > > > > > > >> @@ -2119,34 +2173,59 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags, > >> int ret; > >> > >> #if defined(__APPLE__) && defined(__MACH__) > >> - const char *filename = qdict_get_str(options, "filename"); > > > > [snip] > > > >> + char filename[MAXPATHLEN]; > >> + bool error_occurred = false; > >> + snprintf(filename, MAXPATHLEN, "%s", qdict_get_str(options, "filename")); > > > > This is a step backwards compared to the original code > > IMHO. There's no good reason to use a stack allocated > > fixed array here - the code that follows would be > > quite happy with the original 'const char *'. > > Having to decide when and where to free memory is eliminated with > automatic variables. The code looks cleaner without all the g_free()'s. > It also might eliminate possible memory leaks. There was/is no leak because it qdict_get_str() returns 'const char *' and so nothing needs freeing. So your change is still a backwards steps IMHO. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|