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 <programmingk...@gmail.com> > >> > >> --- > >> 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 :|
