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

[Programmingkid]

On Feb 2, 2016, at 12:31 PM, Daniel P. Berrange wrote:

> 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.

char filename[MAXPATHLEN];
snprintf(filename, MAXPATHLEN, "%s", qdict_get_str(options, "filename"));

So you want the above to be changed so that it goes back to this:

const char *filename = qdict_get_str(options, "filename");

then this code would have to be changed:

snprintf(filename, MAXPATHLEN, "%s", bsd_path);
qdict_put(options, "filename", qstring_from_str(filename));

I could change it to this:

qdict_put(options, "filename", qstring_from_str(bsd_path));

If I did that, then this part would not be accurate anymore:

if (strncmp(filename, "/dev/", 5) == 0) {
            print_unmounting_directions(filename);
            return -1;
        }

filename would be just "/dev/cdrom" for when the user uses the optical drive. 
This would print incorrect unmounting directions. 

I could add another variable that keeps track of the real device name, but that 
would consume more memory. It is so much easier and simpler to just use the 
fixed array.