On Nov 25, 2015, at 11:23 PM, Eric Blake wrote: > On 11/25/2015 09:10 PM, Programmingkid wrote: >> 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. >> >> Signed-off-by: John Arbuckle <programmingk...@gmail.com> >> >> --- >> Added DVD support - real DVD media can now be used in QEMU! >> Changed name of FindEjectableCDMedia to FindEjectableOpticalMedia. >> Added mediaType parameter to FindEjectableOpticalMedia() for media >> indentification. >> Removed unneeded FindEjectableCDMedia() prototype. >> FindEjectableOpticalMedia() now searches for both DVD's and CD's. >> > >> +++ b/block/raw-posix.c >> @@ -42,9 +42,9 @@ >> #include <IOKit/storage/IOMediaBSDClient.h> >> #include <IOKit/storage/IOMedia.h> >> #include <IOKit/storage/IOCDMedia.h> >> -//#include <IOKit/storage/IOCDTypes.h> >> +#include <IOKit/storage/IODVDMedia.h> >> #include <CoreFoundation/CoreFoundation.h> >> -#endif >> +#endif /* (__APPLE__) && (__MACH__) */ >> > > I don't know if my review of v8 crossed your posting of this patch, but > I suggested that this hunk belongs in its own patch.
It is now a related change that the code in the patch depends on. > >> #ifdef __sun__ >> #define _POSIX_PTHREAD_SEMANTICS 1 >> @@ -1975,32 +1975,44 @@ BlockDriver bdrv_file = { >> /* host device */ >> >> #if defined(__APPLE__) && defined(__MACH__) >> -static kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator ); >> static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath, >> CFIndex maxPathSize, int flags); >> -kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator ) >> +static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator, >> + char >> *mediaType) > > Unusual indentation; more typical is: > > | static kern_return_t FindEjectableOpticalMedia(io_iterator_t > *mediaIterator, > | char *mediatType) I agree. I wanted the second long to be right justified with the 80 character line count. > > >> + int index; >> + for (index = 0; index < ARRAY_SIZE(matching_array); index++) { >> + classesToMatch = IOServiceMatching(matching_array[index]); >> + if (classesToMatch == NULL) { >> + printf("IOServiceMatching returned a NULL dictionary.\n"); > > Leftover debugging? It is actually how the function was originally written. It probably needs to be improved. Should I replace printf() with error_report()? > >> + } else { >> + CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey), > > Missing indentation. Fixed in the next patch. > >> + >> kCFBooleanTrue); >> + } >> + kernResult = IOServiceGetMatchingServices(masterPort, >> classesToMatch, >> + >> mediaIterator); > > More unusual indentation. Fixed in the next patch. > >> + if (KERN_SUCCESS != kernResult) { >> + printf("IOServiceGetMatchingServices returned %d\n", >> kernResult); >> + } >> >> + /* If you found a match, leave the loop */ >> + if (*mediaIterator != 0) { >> + DPRINTF("Matching using %s\n", matching_array[index]); >> + snprintf(mediaType, strlen(matching_array[index])+1, "%s", > > Spaces around binary '+'. What's wrong with no spaces around the plus sign? > >> + /* if a working partition on the device was not found */ >> + if (partition_found == false) { >> + error_setg(errp, "Error: Failed to find a working partition on " >> + >> "disc!\n"); > > and I already pointed out on v8 that this is not the correct usage of > error_setg(). So, here's hoping v10 addresses the comments here and > elsewhere. Kevin Wolf wanted it this way. What would you do instead? > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org > Thank you very much for reviewing my patches.