| Message ID | F65606C3-58F6-4A5B-A8BD-554C64011270@gmail.com |
|---|---|
| State | New |
| Headers | show |
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 <programmingkidx@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. > #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) > + 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? > + } else { > + CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey), Missing indentation. > + kCFBooleanTrue); > + } > + kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch, > + mediaIterator); More unusual indentation. > + 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 '+'. > + /* 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.
On 11/25/2015 09:23 PM, Eric Blake wrote: >> +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) And then my mailer messes it up :( > static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator, > char *mediatType) Let's see if that's better (the 'char' is directly beneath the 'io_iterator_t').
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 <programmingkidx@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.
On Nov 25, 2015, at 11:29 PM, Eric Blake wrote: > On 11/25/2015 09:23 PM, Eric Blake wrote: > >>> +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) > > And then my mailer messes it up :( > >> static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator, >> char *mediatType) > > Let's see if that's better (the 'char' is directly beneath the > 'io_iterator_t'). In my email program, the 'char' appears underneath the Ejectable word. When I change the font to monaco (A mono-spaced font), the 'char' does appear underneath the io_iterator_t.
On 11/27/2015 12:35 PM, Programmingkid wrote: >> 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. No. We don't right-justify code to 80 columns. That's not how it is done. Trying to do it just makes you look like the proverbial 'kid' in your pseudonym, rather than an adult to be taken seriously. Really, PLEASE follow the indentation patterns of the rest of the code base - where continued lines are left-justified to be underneath the character after (, and NOT right-justified to 80 columns. Violating style doesn't make your code invalid, but does make your patches less likely to be applied. >>> + /* 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? Again, the prevailing conventions in the code base is that you put spaces around every binary operator. Yes, there is existing old code that does not meet the conventions, but it is not an excuse to add new code that is gratuitously different. > >> >>> + /* 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? Keven and I both want you to use error_setg(), but to use it correctly - and the correct way is to NOT supply a trailing \n. > > Thank you very much for reviewing my patches. The least you can do for showing that gratitude is to actually improve your next revisions along the lines of the comments you have been given. Quit making it feel like pulling teeth just to get your patches to match the coding conventions prevalent in the project.
Am 30.11.2015 um 17:19 hat Eric Blake geschrieben: > On 11/27/2015 12:35 PM, Programmingkid wrote: > > >> 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. > > No. We don't right-justify code to 80 columns. That's not how it is > done. Trying to do it just makes you look like the proverbial 'kid' in > your pseudonym, rather than an adult to be taken seriously. > > Really, PLEASE follow the indentation patterns of the rest of the code > base - where continued lines are left-justified to be underneath the > character after (, and NOT right-justified to 80 columns. Violating > style doesn't make your code invalid, but does make your patches less > likely to be applied. > > > >>> + /* 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? > > Again, the prevailing conventions in the code base is that you put > spaces around every binary operator. Yes, there is existing old code > that does not meet the conventions, but it is not an excuse to add new > code that is gratuitously different. > > > > >> > >>> + /* 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? > > Keven and I both want you to use error_setg(), but to use it correctly - > and the correct way is to NOT supply a trailing \n. Nor leading "Error:", for that matter. Kevin
On Nov 30, 2015, at 11:26 AM, Kevin Wolf wrote: > Am 30.11.2015 um 17:19 hat Eric Blake geschrieben: >> On 11/27/2015 12:35 PM, Programmingkid wrote: >> >>>> 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. >> >> No. We don't right-justify code to 80 columns. That's not how it is >> done. Trying to do it just makes you look like the proverbial 'kid' in >> your pseudonym, rather than an adult to be taken seriously. >> >> Really, PLEASE follow the indentation patterns of the rest of the code >> base - where continued lines are left-justified to be underneath the >> character after (, and NOT right-justified to 80 columns. Violating >> style doesn't make your code invalid, but does make your patches less >> likely to be applied. >> >> >>>>> + /* 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? >> >> Again, the prevailing conventions in the code base is that you put >> spaces around every binary operator. Yes, there is existing old code >> that does not meet the conventions, but it is not an excuse to add new >> code that is gratuitously different. >> >>> >>>> >>>>> + /* 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? >> >> Keven and I both want you to use error_setg(), but to use it correctly - >> and the correct way is to NOT supply a trailing \n. > > Nor leading "Error:", for that matter. I just think that using "Error" does communicate the fact that something is wrong a lot better than just printing the message.
On 11/30/2015 09:38 AM, Programmingkid wrote: >>>>>> + /* 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? >>> >>> Keven and I both want you to use error_setg(), but to use it correctly - >>> and the correct way is to NOT supply a trailing \n. >> >> Nor leading "Error:", for that matter. > > I just think that using "Error" does communicate the fact that something is wrong > a lot better than just printing the message. But error_setg() _already_ provides the context that an error message is being printed. The whole point of using wrapper functions is that the common functionality (like an 'error:' prefix, or '\n' suffix) is done in the wrapper, not at every call site. If you were using raw printf(), then yes, using your own 'Error:' prefix would be appropriate. But we aren't using raw printf(). Your use of an 'Error:' prefix is therefore redundant, and we are trying to convince you that you are using error_setg() incorrectly.
diff --git a/block/raw-posix.c b/block/raw-posix.c index ccfec1c..a11a9e7 100644 --- a/block/raw-posix.c +++ 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__) */ #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) { kern_return_t kernResult; mach_port_t masterPort; CFMutableDictionaryRef classesToMatch; + const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass}; kernResult = IOMasterPort( MACH_PORT_NULL, &masterPort ); if ( KERN_SUCCESS != kernResult ) { printf( "IOMasterPort returned %d\n", kernResult ); } - classesToMatch = IOServiceMatching( kIOCDMediaClass ); - if ( classesToMatch == NULL ) { - printf( "IOServiceMatching returned a NULL dictionary.\n" ); - } else { - CFDictionarySetValue( classesToMatch, CFSTR( kIOMediaEjectableKey ), kCFBooleanTrue ); - } - kernResult = IOServiceGetMatchingServices( masterPort, classesToMatch, mediaIterator ); - if ( KERN_SUCCESS != kernResult ) - { - printf( "IOServiceGetMatchingServices returned %d\n", kernResult ); - } + 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"); + } else { + CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey), + kCFBooleanTrue); + } + kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch, + mediaIterator); + 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", + matching_array[index]); + break; + } + } return kernResult; } @@ -2033,7 +2045,36 @@ kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath, return kernResult; } -#endif +/* Sets up a real cdrom for use in QEMU */ +static bool setup_cdrom(char *bsd_path, Error **errp) +{ + int index, num_of_test_partitions = 2, fd; + char test_partition[MAXPATHLEN]; + bool partition_found = false; + + /* look for a working partition */ + for (index = 0; index < num_of_test_partitions; index++) { + snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path, + index); + fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE); + if (fd >= 0) { + partition_found = true; + qemu_close(fd); + break; + } + } + + /* 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"); + } else { + DPRINTF("Using %s as optical disc\n", test_partition); + pstrcpy(bsd_path, MAXPATHLEN, test_partition); + } + return partition_found; +} +#endif /* defined(__APPLE__) && defined(__MACH__) */ static int hdev_probe_device(const char *filename) { @@ -2115,6 +2156,17 @@ static bool hdev_is_sg(BlockDriverState *bs) return false; } +/* Prints directions on mounting and unmounting a device */ +static void print_unmounting_directions(const char *file_name) +{ + error_report("Error: If device %s is mounted on the desktop, unmount" + " it first before using it in QEMU.\n", file_name); + error_report("Command to unmount device: diskutil unmountDisk %s\n", + file_name); + error_report("Command to mount device: diskutil mountDisk %s\n", + file_name); +} + static int hdev_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { @@ -2125,30 +2177,33 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags, #if defined(__APPLE__) && defined(__MACH__) const char *filename = qdict_get_str(options, "filename"); - if (strstart(filename, "/dev/cdrom", NULL)) { - kern_return_t kernResult; + /* If using a real cdrom */ + if (strcmp(filename, "/dev/cdrom") == 0) { + char bsd_path[MAXPATHLEN]; + char mediaType[11]; /* IODVDMedia is the longest value */ io_iterator_t mediaIterator; - char bsdPath[ MAXPATHLEN ]; - int fd; - - kernResult = FindEjectableCDMedia( &mediaIterator ); - kernResult = GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath), - flags); - if ( bsdPath[ 0 ] != '\0' ) { - strcat(bsdPath,"s0"); - /* some CDs don't have a partition 0 */ - fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); - if (fd < 0) { - bsdPath[strlen(bsdPath)-1] = '1'; - } else { - qemu_close(fd); - } - filename = bsdPath; - qdict_put(options, "filename", qstring_from_str(filename)); + FindEjectableOpticalMedia(&mediaIterator, mediaType); + GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags); + if (mediaIterator) { + IOObjectRelease(mediaIterator); } - if ( mediaIterator ) - IOObjectRelease( mediaIterator ); + /* If a real optical drive was not found */ + if (bsd_path[0] == '\0') { + error_setg(errp, "Error: failed to obtain bsd path for optical" + " drive!\n"); + return -1; + } + + /* If using a cdrom disc and finding a partition on the disc failed */ + if (strncmp(mediaType, "IOCDMedia", 9) == 0 && + setup_cdrom(bsd_path, errp) == false) { + print_unmounting_directions(bsd_path); + return -1; + } + + filename = bsd_path; + qdict_put(options, "filename", qstring_from_str(filename)); } #endif @@ -2159,9 +2214,16 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags, if (local_err) { error_propagate(errp, local_err); } - return ret; } +#if defined(__APPLE__) && defined(__MACH__) + /* if a physical device experienced an error while being opened */ + if (strncmp(filename, "/dev/", 5) == 0 && ret != 0) { + print_unmounting_directions(filename); + return -1; + } +#endif /* defined(__APPLE__) && defined(__MACH__) */ + /* Since this does ioctl the device must be already opened */ bs->sg = hdev_is_sg(bs);
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 <programmingkidx@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. block/raw-posix.c | 138 ++++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 100 insertions(+), 38 deletions(-)