Re: [Qemu-devel] ping: [PATCH v13] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Daniel P. Berrange]

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 <address@hidden>
> >> 
> >> ---
> >> 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 :|