Re: [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: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.

> 
>> +    /* If using a real cdrom */
>> +    if (strcmp(filename, "/dev/cdrom") == 0) {
>> +        char bsd_path[MAXPATHLEN];
>> +        char *mediaType = NULL;
>> +        kern_return_t ret_val;
>> +        io_iterator_t mediaIterator = 0;
>> +
>> +        mediaType = FindEjectableOpticalMedia(&mediaIterator);
>> +        if (mediaType == NULL) {
>> +            error_setg(errp, "Please make sure your CD/DVD is in the 
>> optical"
>> +                       " drive");
>> +            error_occurred = true;
>> +            goto hdev_open_Mac_error;
>> +        }
>> +
>> +        ret_val = GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), 
>> flags);
>> +        if (ret_val != KERN_SUCCESS) {
>> +            error_setg(errp, "Could not get BSD path for optical drive");
>> +            error_occurred = true;
>> +            goto hdev_open_Mac_error;
>> +        }
>> +
>> +        /* If a real optical drive was not found */
>> +        if (bsd_path[0] == '\0') {
>> +            error_setg(errp, "Failed to obtain bsd path for optical drive");
>> +            error_occurred = true;
>> +            goto hdev_open_Mac_error;
>> +        }
>> +
>> +        /* If using a cdrom disc and finding a partition on the disc failed 
>> */
>> +        if (strncmp(mediaType, kIOCDMediaClass, 9) == 0 &&
>> +            setup_cdrom(bsd_path, errp) == false) {
>> +            print_unmounting_directions(bsd_path);
>> +            error_occurred = true;
>> +            goto hdev_open_Mac_error;
>>         }
>> 
>> -        if ( mediaIterator )
>> -            IOObjectRelease( mediaIterator );
>> +        snprintf(filename, MAXPATHLEN, "%s", bsd_path);
>> +        qdict_put(options, "filename", qstring_from_str(filename));
>> +
>> +hdev_open_Mac_error:
>> +        g_free(mediaType);
>> +        if (mediaIterator) {
>> +            IOObjectRelease(mediaIterator);
>> +        }
>> +        if (error_occurred) {
>> +            return -1;
>> +        }
>>     }
>> -#endif
>> +#endif /* defined(__APPLE__) && defined(__MACH__) */
> 
> Regards,
> Daniel

Thank you for your input.