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.
>
>> + /* 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.
