On Thu, Dec 10, 2015 at 09:39:51AM -0500, Programmingkid wrote:
> https://patchwork.ozlabs.org/patch/550295/
>
> 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.
>
This commit message doesn't seem to really match the patch; it is more
than just a message displayed to the user. For instance, before it
looked for just kIOCDMediaClass - now, it searches for kIOCDMediaClass
and kIODVDMediaClass.
> Signed-off-by: John Arbuckle <programmingk...@gmail.com>
>
> ---
> error_report()'s had their \n, '.', and "Error:" removed.
> Indentations are now at the left parenthesis rather than
> at the 80 character mark.
> Added spaces between the + sign.
>
Also, this patch seems garbled. When saved via Mutt, or when I view
it "raw", there are '=20" and '=3D' all around, a sure sign that it is
(or once was) not plaintext.
I recommend using git format-patch [1] and git send-email [1] to
create and send patches - it'll do the Right Thing.
> block/raw-posix.c | 135
> +++++++++++++++++++++++++++++++++++++++--------------
> 1 files changed, 99 insertions(+), 36 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index ccfec1c..39e523b 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -43,6 +43,7 @@
> #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
>
> @@ -1975,32 +1976,46 @@ 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) {
> + error_report("IOServiceMatching returned NULL for %s",
> + matching_array[index]);
> + continue;
> + }
> + CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
> + kCFBooleanTrue);
> + kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
> + mediaIterator);
> + if (kernResult != KERN_SUCCESS) {
> + error_report("Note: IOServiceGetMatchingServices returned %d",
> + kernResult);
> + }
>
> + /* If a match was found, leave the loop */
> + if (*mediaIterator != 0) {
Since mediaIterator was not ever initialized in hdev_open, we can't
assume the value of mediaIterator as being 0 after kernResult !=
KERN_SUCCESS, can we? A quick google search [3] shows it ambiguous, so
best initialize mediaIterator down below...
> + DPRINTF("Matching using %s\n", matching_array[index]);
> + snprintf(mediaType, strlen(matching_array[index]) + 1, "%s",
> + matching_array[index]);
Just use g_strdup().
We use snprintf here, with a size limit of the string referenced in
the array, which references a string defined in an OS X system header...
> + break;
> + }
> + }
> return kernResult;
You may return garbage here, because kernResult is never initialized,
and your for() loop short circuits on a NULL return from
IOServiceMatching(). Does this compile with our flags? I don't have
OS X so I can't try it, but I thought at least with gcc and -werror, a
possible uninitialized usage would be flagged.
> }
>
> @@ -2033,7 +2048,35 @@ 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;
What is the magic of 2 test partitions?
> + 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, "Failed to find a working partition on disc");
> + } else {
> + DPRINTF("Using %s as optical disc\n", test_partition);
> + pstrcpy(bsd_path, MAXPATHLEN, test_partition);
In OS X, is MAXPATHLEN including the terminating NULL?
> + }
> + return partition_found;
> +}
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
>
> static int hdev_probe_device(const char *filename)
> {
> @@ -2115,6 +2158,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("If device %s is mounted on the desktop, unmount"
> + " it first before using it in QEMU", file_name);
> + error_report("Command to unmount device: diskutil unmountDisk %s",
> + file_name);
> + error_report("Command to mount device: diskutil mountDisk %s",
> + file_name);
> +}
> +
> static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
> Error **errp)
> {
> @@ -2125,30 +2179,32 @@ 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 */
... yet here we just hard code the array size to 11, which is prone to
an error later on by either a change in the system header (not very
likely, but possible), or by expanding the media types we support in
the future.
Just bypass that fragility, and use g_strdup() (and later g_free()),
so we are impervious to the exact string size. You'll likely want a
common exit for the g_free() call.
> io_iterator_t mediaIterator;
...Given your usage of mediaIterator, I think you need to initialize it
to 0 here.
> - 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);
Return value ignored here, don't ignore it.
> + GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags);
Return value ignored here, don't ignore it.
> + if (mediaIterator) {
> + IOObjectRelease(mediaIterator);
> }
>
> - if ( mediaIterator )
> - IOObjectRelease( mediaIterator );
> + /* If a real optical drive was not found */
> + if (bsd_path[0] == '\0') {
> + error_setg(errp, "Failed to obtain bsd path for optical drive");
> + 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 +2215,16 @@ static int hdev_open(BlockDriverState *bs, QDict
> *options, int flags,
> if (local_err) {
> error_propagate(errp, local_err);
> }
> - return ret;
Spurious line delete?
> }
>
> +#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);
>
> --
> 1.7.5.4
>
>
>
[1] https://git-scm.com/docs/git-format-patch
[2] https://git-scm.com/docs/git-send-email
[3]
https://developer.apple.com/library/mac/documentation/IOKit/Reference/IOKitLib_header_reference/index.html#//apple_ref/doc/c_ref/IOServiceGetMatchingServices
