* [Qemu-devel] ping [PATCH v11] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
@ 2015-12-10 14:39 Programmingkid
2015-12-11 22:00 ` [Qemu-devel] [Qemu-block] " Jeff Cody
0 siblings, 1 reply; 10+ messages in thread
From: Programmingkid @ 2015-12-10 14:39 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel qemu-devel, Qemu-block
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.
Signed-off-by: John Arbuckle <programmingkidx@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.
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) {
+ DPRINTF("Matching using %s\n", matching_array[index]);
+ snprintf(mediaType, strlen(matching_array[index]) + 1, "%s",
+ matching_array[index]);
+ break;
+ }
+ }
return kernResult;
}
@@ -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;
+ 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);
+ }
+ 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 */
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, "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;
}
+#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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] ping [PATCH v11] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2015-12-10 14:39 [Qemu-devel] ping [PATCH v11] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host Programmingkid
@ 2015-12-11 22:00 ` Jeff Cody
2015-12-11 23:40 ` Programmingkid
2015-12-12 3:27 ` [Qemu-devel] [PATCH v12] " Programmingkid
0 siblings, 2 replies; 10+ messages in thread
From: Jeff Cody @ 2015-12-11 22:00 UTC (permalink / raw)
To: Programmingkid; +Cc: Kevin Wolf, qemu-devel qemu-devel, Qemu-block
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 <programmingkidx@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] ping [PATCH v11] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2015-12-11 22:00 ` [Qemu-devel] [Qemu-block] " Jeff Cody
@ 2015-12-11 23:40 ` Programmingkid
2015-12-11 23:53 ` Eric Blake
2015-12-12 3:27 ` [Qemu-devel] [PATCH v12] " Programmingkid
1 sibling, 1 reply; 10+ messages in thread
From: Programmingkid @ 2015-12-11 23:40 UTC (permalink / raw)
To: Jeff Cody; +Cc: Kevin Wolf, qemu-devel qemu-devel, Qemu-block
On Dec 11, 2015, at 5:00 PM, Jeff Cody wrote:
> 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.
I guess the commit message is a little out of date. How about this:
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. Also new to this patch is the ability to use DVD
disc.
>
>> Signed-off-by: John Arbuckle <programmingkidx@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.
You really see that? I don't know why. The link I provide to the patch in
patchworks shows no problems. The patch itself was sent as a plain text
file.
>
>> 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...
Sounds like a good suggestion.
>
>
>> + DPRINTF("Matching using %s\n", matching_array[index]);
>> + snprintf(mediaType, strlen(matching_array[index]) + 1, "%s",
>> + matching_array[index]);
>
> Just use g_strdup().
I like snprintf() because it very well documented.
Is this what you had in mind:
mediaType = g_strdup(matching_array[index]);
>
> 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().
Tests show the code works. I did do tests where there was no CD in the
DVD drive. The code does gracefully handle the situation.
> Does this compile with our flags?
Yes
> 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.
Didn't have any warnings or errors compiling.
>
>> }
>>
>> @@ -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?
CD's are setup one way or another. My tests show some CD's
only work when we use partition zero. Other times it is
partition one.
>
>> + 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?
I think so.
>
>> + }
>> + 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.
I will use g_strdup() on my next patch.
>
>> io_iterator_t mediaIterator;
>
> ...Given your usage of mediaIterator, I think you need to initialize it
> to 0 here.
Ok
>
>> - 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.
ok
>
>> + GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags);
>
> Return value ignored here, don't ignore it.
ok
>
>> + 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?
It needed to go so the print_unmounting_directions()
function can have a chance to be called. I do see a
need for it in Linux, so I will bring it back in the
next patch.
>
>> }
>>
>> +#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
Thank you for reviewing my patch.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] ping [PATCH v11] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2015-12-11 23:40 ` Programmingkid
@ 2015-12-11 23:53 ` Eric Blake
0 siblings, 0 replies; 10+ messages in thread
From: Eric Blake @ 2015-12-11 23:53 UTC (permalink / raw)
To: Programmingkid, Jeff Cody; +Cc: Kevin Wolf, qemu-devel qemu-devel, Qemu-block
[-- Attachment #1: Type: text/plain, Size: 3233 bytes --]
On 12/11/2015 04:40 PM, Programmingkid wrote:
> I guess the commit message is a little out of date. How about this:
>
> 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. Also new to this patch is the ability to use DVD
> disc.
>
The use of "Also" in a commit message is often a sign of trying to do
too much in one patch. If you are doing two distinct things (adding a
new message, vs. adding the ability to use a DVD disk), then two patches
is probably the best way to approach it.
>> 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.
>
> You really see that? I don't know why. The link I provide to the patch in
> patchworks shows no problems. The patch itself was sent as a plain text
> file.
Your mail was sent with:
> Content-Type: text/plain; charset=us-ascii
> Content-Transfer-Encoding: quoted-printable
> Mime-Version: 1.0 (Apple Message framework v1084)
and the body indeed contains quoted-printable text, such as:
> @@ -1975,32 +1976,46 @@ BlockDriver bdrv_file =3D {
> /* host device */
> =20
Many mailers are good enough to re-render things on the fly
(Thunderbird, for example, does not have those annoying =3D and =20
insertions in the displayed text) - but what gets rendered in the mail
client is different than what the mail client saves to file.
Mails sent by 'git send-email', on the other hand, use:
> MIME-Version: 1.0
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
Apparently, mutt doesn't quite defang a quoted-printable message
properly, and/or 'git am' doesn't take too well to a quoted-printable
message when trying to apply a saved file as a patch. Which is all the
more reason why using the same tool as everyone else makes sure that
your patches will interoperate with the receivers' tools the same as the
receiver is already used to handling other patches.
>> Just use g_strdup().
>
> I like snprintf() because it very well documented.
So is g_strdup(). But more important than being well-documented,
g_strdup() also has the benefit of being correctly usable with less
boilerplate - you can accomplish the same task in fewer lines of code,
and focus the reviewer on your task rather than on the boilerplate.
That, and I've seen a number of programs that incorrectly use snprintf()
(such as incorrectly sizing a buffer, or failing to check for sizing
errors after the fact). An interface that is hard to use correctly
should be avoided in favor of one that is less mental effort.
>
> Is this what you had in mind:
>
> mediaType = g_strdup(matching_array[index]);
Pretty much, coupled with g_free(mediaType) later on.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] [PATCH v12] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2015-12-11 22:00 ` [Qemu-devel] [Qemu-block] " Jeff Cody
2015-12-11 23:40 ` Programmingkid
@ 2015-12-12 3:27 ` Programmingkid
2015-12-18 23:58 ` [Qemu-devel] ping " Programmingkid
2016-01-18 16:14 ` [Qemu-devel] " Kevin Wolf
1 sibling, 2 replies; 10+ messages in thread
From: Programmingkid @ 2015-12-12 3:27 UTC (permalink / raw)
To: Jeff Cody; +Cc: Kevin Wolf, qemu-devel qemu-devel, Qemu-block
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 <programmingkidx@gmail.com>
---
Removed mediaType parameter from FindEjectableOpticalMedia().
Added goto statements to hdev_open.
Replaced snprintf() with g_strdup() in FindEjectableOpticalMedia().
Put back return statement in hdev_open for Linux compatibility.
block/raw-posix.c | 163 ++++++++++++++++++++++++++++++++++++++++-------------
1 files changed, 124 insertions(+), 39 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index d9162fd..82e8e62 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,33 +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 char *FindEjectableOpticalMedia(io_iterator_t *mediaIterator)
{
- kern_return_t kernResult;
+ kern_return_t kernResult = KERN_FAILURE;
mach_port_t masterPort;
CFMutableDictionaryRef classesToMatch;
+ const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
+ char *mediaType = NULL;
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);
+ }
- return kernResult;
+ /* If a match was found, leave the loop */
+ if (*mediaIterator != 0) {
+ DPRINTF("Matching using %s\n", matching_array[index]);
+ mediaType = g_strdup(matching_array[index]);
+ break;
+ }
+ }
+ return mediaType;
}
kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
@@ -2033,7 +2047,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;
+ 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);
+ }
+ return partition_found;
+}
+#endif /* defined(__APPLE__) && defined(__MACH__) */
static int hdev_probe_device(const char *filename)
{
@@ -2115,6 +2157,16 @@ 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,32 +2177,55 @@ 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;
- 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));
+ /* 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");
+ 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");
+ 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");
+ goto hdev_open_Mac_error;
+ }
+
+ /* 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);
+ goto hdev_open_Mac_error;
}
- if ( mediaIterator )
- IOObjectRelease( mediaIterator );
+ g_free(mediaType);
+ filename = bsd_path;
+ qdict_put(options, "filename", qstring_from_str(filename));
+ goto continue_as_normal; /* skip over error handling code */
+
+ /* If an error occurred above */
+ hdev_open_Mac_error:
+ if (mediaIterator) {
+ IOObjectRelease(mediaIterator);
+ }
+ g_free(mediaType);
+ return -1;
+
+ continue_as_normal: ;
}
-#endif
+#endif /* defined(__APPLE__) && defined(__MACH__) */
s->type = FTYPE_FILE;
@@ -2159,8 +2234,18 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
if (local_err) {
error_propagate(errp, local_err);
}
+ #if !defined(__APPLE__) && !defined(__MACH__)
return ret;
+ #endif /* !defined(__APPLE__) && !defined(__MACH__) */
+ }
+
+#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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [Qemu-devel] ping Re: [PATCH v12] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2015-12-12 3:27 ` [Qemu-devel] [PATCH v12] " Programmingkid
@ 2015-12-18 23:58 ` Programmingkid
2015-12-29 0:27 ` Programmingkid
2016-01-18 16:14 ` [Qemu-devel] " Kevin Wolf
1 sibling, 1 reply; 10+ messages in thread
From: Programmingkid @ 2015-12-18 23:58 UTC (permalink / raw)
To: Kevin Wolf, qemu-devel qemu-devel, Qemu-block
https://patchwork.ozlabs.org/patch/555945/
On Dec 11, 2015, at 10:27 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. Now QEMU uses both CD and DVD media.
>
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>
> ---
> Removed mediaType parameter from FindEjectableOpticalMedia().
> Added goto statements to hdev_open.
> Replaced snprintf() with g_strdup() in FindEjectableOpticalMedia().
> Put back return statement in hdev_open for Linux compatibility.
>
> block/raw-posix.c | 163 ++++++++++++++++++++++++++++++++++++++++-------------
> 1 files changed, 124 insertions(+), 39 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index d9162fd..82e8e62 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,33 +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 char *FindEjectableOpticalMedia(io_iterator_t *mediaIterator)
> {
> - kern_return_t kernResult;
> + kern_return_t kernResult = KERN_FAILURE;
> mach_port_t masterPort;
> CFMutableDictionaryRef classesToMatch;
> + const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
> + char *mediaType = NULL;
>
> 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);
> + }
>
> - return kernResult;
> + /* If a match was found, leave the loop */
> + if (*mediaIterator != 0) {
> + DPRINTF("Matching using %s\n", matching_array[index]);
> + mediaType = g_strdup(matching_array[index]);
> + break;
> + }
> + }
> + return mediaType;
> }
>
> kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
> @@ -2033,7 +2047,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;
> + 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);
> + }
> + return partition_found;
> +}
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
>
> static int hdev_probe_device(const char *filename)
> {
> @@ -2115,6 +2157,16 @@ 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,32 +2177,55 @@ 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;
> - 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));
> + /* 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");
> + 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");
> + 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");
> + goto hdev_open_Mac_error;
> + }
> +
> + /* 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);
> + goto hdev_open_Mac_error;
> }
>
> - if ( mediaIterator )
> - IOObjectRelease( mediaIterator );
> + g_free(mediaType);
> + filename = bsd_path;
> + qdict_put(options, "filename", qstring_from_str(filename));
> + goto continue_as_normal; /* skip over error handling code */
> +
> + /* If an error occurred above */
> + hdev_open_Mac_error:
> + if (mediaIterator) {
> + IOObjectRelease(mediaIterator);
> + }
> + g_free(mediaType);
> + return -1;
> +
> + continue_as_normal: ;
> }
> -#endif
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
>
> s->type = FTYPE_FILE;
>
> @@ -2159,8 +2234,18 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
> if (local_err) {
> error_propagate(errp, local_err);
> }
> + #if !defined(__APPLE__) && !defined(__MACH__)
> return ret;
> + #endif /* !defined(__APPLE__) && !defined(__MACH__) */
> + }
> +
> +#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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Qemu-devel] ping Re: [PATCH v12] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2015-12-18 23:58 ` [Qemu-devel] ping " Programmingkid
@ 2015-12-29 0:27 ` Programmingkid
2016-01-04 16:35 ` [Qemu-devel] [Qemu-block] " Max Reitz
0 siblings, 1 reply; 10+ messages in thread
From: Programmingkid @ 2015-12-29 0:27 UTC (permalink / raw)
To: Kevin Wolf, qemu-devel qemu-devel, Qemu-block
I do realize you are busy Kevin, but I would
appreciate knowing my patch is in line
for review.
https://patchwork.ozlabs.org/patch/555945/
On Dec 11, 2015, at 10:27 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. Now QEMU uses both CD and DVD media.
>
> Signed-off-by: John Arbuckle <programmingkidx@gmail.com>
>
> ---
> Removed mediaType parameter from FindEjectableOpticalMedia().
> Added goto statements to hdev_open.
> Replaced snprintf() with g_strdup() in FindEjectableOpticalMedia().
> Put back return statement in hdev_open for Linux compatibility.
>
> block/raw-posix.c | 163 ++++++++++++++++++++++++++++++++++++++++-------------
> 1 files changed, 124 insertions(+), 39 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index d9162fd..82e8e62 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,33 +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 char *FindEjectableOpticalMedia(io_iterator_t *mediaIterator)
> {
> - kern_return_t kernResult;
> + kern_return_t kernResult = KERN_FAILURE;
> mach_port_t masterPort;
> CFMutableDictionaryRef classesToMatch;
> + const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
> + char *mediaType = NULL;
>
> 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);
> + }
>
> - return kernResult;
> + /* If a match was found, leave the loop */
> + if (*mediaIterator != 0) {
> + DPRINTF("Matching using %s\n", matching_array[index]);
> + mediaType = g_strdup(matching_array[index]);
> + break;
> + }
> + }
> + return mediaType;
> }
>
> kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
> @@ -2033,7 +2047,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;
> + 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);
> + }
> + return partition_found;
> +}
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
>
> static int hdev_probe_device(const char *filename)
> {
> @@ -2115,6 +2157,16 @@ 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,32 +2177,55 @@ 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;
> - 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));
> + /* 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");
> + 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");
> + 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");
> + goto hdev_open_Mac_error;
> + }
> +
> + /* 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);
> + goto hdev_open_Mac_error;
> }
>
> - if ( mediaIterator )
> - IOObjectRelease( mediaIterator );
> + g_free(mediaType);
> + filename = bsd_path;
> + qdict_put(options, "filename", qstring_from_str(filename));
> + goto continue_as_normal; /* skip over error handling code */
> +
> + /* If an error occurred above */
> + hdev_open_Mac_error:
> + if (mediaIterator) {
> + IOObjectRelease(mediaIterator);
> + }
> + g_free(mediaType);
> + return -1;
> +
> + continue_as_normal: ;
> }
> -#endif
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
>
> s->type = FTYPE_FILE;
>
> @@ -2159,8 +2234,18 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
> if (local_err) {
> error_propagate(errp, local_err);
> }
> + #if !defined(__APPLE__) && !defined(__MACH__)
> return ret;
> + #endif /* !defined(__APPLE__) && !defined(__MACH__) */
> + }
> +
> +#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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] ping Re: [PATCH v12] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2015-12-29 0:27 ` Programmingkid
@ 2016-01-04 16:35 ` Max Reitz
2016-01-04 16:57 ` Programmingkid
0 siblings, 1 reply; 10+ messages in thread
From: Max Reitz @ 2016-01-04 16:35 UTC (permalink / raw)
To: Programmingkid, Kevin Wolf, qemu-devel qemu-devel, Qemu-block
[-- Attachment #1: Type: text/plain, Size: 296 bytes --]
On 29.12.2015 01:27, Programmingkid wrote:
> I do realize you are busy Kevin, but I would
> appreciate knowing my patch is in line
> for review.
Primarily, he's been on holiday since before christmas until next week.
(I'm telling you so you don't wonder why nothing happens.)
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [Qemu-block] ping Re: [PATCH v12] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2016-01-04 16:35 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2016-01-04 16:57 ` Programmingkid
0 siblings, 0 replies; 10+ messages in thread
From: Programmingkid @ 2016-01-04 16:57 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel qemu-devel, Qemu-block
On Jan 4, 2016, at 11:35 AM, Max Reitz wrote:
> On 29.12.2015 01:27, Programmingkid wrote:
>> I do realize you are busy Kevin, but I would
>> appreciate knowing my patch is in line
>> for review.
>
> Primarily, he's been on holiday since before christmas until next week.
>
> (I'm telling you so you don't wonder why nothing happens.)
>
> Max
>
Thank you very much. I guess I have been a little frustrated with
this patch. I have been trying to have it submitted into QEMU
since August 2015!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH v12] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2015-12-12 3:27 ` [Qemu-devel] [PATCH v12] " Programmingkid
2015-12-18 23:58 ` [Qemu-devel] ping " Programmingkid
@ 2016-01-18 16:14 ` Kevin Wolf
1 sibling, 0 replies; 10+ messages in thread
From: Kevin Wolf @ 2016-01-18 16:14 UTC (permalink / raw)
To: Programmingkid; +Cc: Jeff Cody, qemu-devel qemu-devel, Qemu-block
Am 12.12.2015 um 04:27 hat Programmingkid geschrieben:
> 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 <programmingkidx@gmail.com>
Please post patches at the top level instead as answers deeply nested in
the email thread of another version of the same series.
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index d9162fd..82e8e62 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,33 +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 char *FindEjectableOpticalMedia(io_iterator_t *mediaIterator)
> {
> - kern_return_t kernResult;
> + kern_return_t kernResult = KERN_FAILURE;
> mach_port_t masterPort;
> CFMutableDictionaryRef classesToMatch;
> + const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
> + char *mediaType = NULL;
>
> 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);
> + }
>
> - return kernResult;
> + /* If a match was found, leave the loop */
> + if (*mediaIterator != 0) {
> + DPRINTF("Matching using %s\n", matching_array[index]);
> + mediaType = g_strdup(matching_array[index]);
> + break;
> + }
It's unclear to me whether *mediaIterator is valid in error cases. The
documentation says "An iterator handle is returned on success, and
should be released by the caller when the iteration is finished", but it
doesn't say anything about error cases.
It feels safer to 'continue;' in the != KERN_SUCCESS case.
> + }
> + return mediaType;
> }
>
> kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
> @@ -2033,7 +2047,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;
> + 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);
> + }
> + return partition_found;
> +}
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
>
> static int hdev_probe_device(const char *filename)
> {
> @@ -2115,6 +2157,16 @@ 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);
> +}
On Linux:
block/raw-posix.c:2150:13: error: 'print_unmounting_directions' defined but not used [-Werror=unused-function]
static void print_unmounting_directions(const char *file_name)
> static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
> Error **errp)
> {
> @@ -2125,32 +2177,55 @@ 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;
> - 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));
> + /* 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");
> + 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");
> + 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");
> + goto hdev_open_Mac_error;
> + }
> +
> + /* 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) {
Indentation is off.
I guess kIOCDMediaClass, as returned by FindEjectableOpticalMedia(), is
the same as "IOCDMedia". You should use the same constant here, then.
> + print_unmounting_directions(bsd_path);
> + goto hdev_open_Mac_error;
> }
>
> - if ( mediaIterator )
> - IOObjectRelease( mediaIterator );
> + g_free(mediaType);
> + filename = bsd_path;
filename is still used after bsd_path goes out of scope.
> + qdict_put(options, "filename", qstring_from_str(filename));
> + goto continue_as_normal; /* skip over error handling code */
> +
> + /* If an error occurred above */
> + hdev_open_Mac_error:
> + if (mediaIterator) {
> + IOObjectRelease(mediaIterator);
> + }
> + g_free(mediaType);
> + return -1;
> +
> + continue_as_normal: ;
What is this? Seriously, you don't expect me to merge _that_, do you?
There is no way I'll accept jumping across code with goto like this.
Error handling belongs at the end of the function. If you need many
variables from a local block, so that pulling them up to function level
would be awkward and doing local error handling is preferable, it may be
an indication that your block should become a function of its own.
> }
> -#endif
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
>
> s->type = FTYPE_FILE;
>
> @@ -2159,8 +2234,18 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
> if (local_err) {
> error_propagate(errp, local_err);
> }
> + #if !defined(__APPLE__) && !defined(__MACH__)
> return ret;
> + #endif /* !defined(__APPLE__) && !defined(__MACH__) */
*sigh*
> + }
> +
> +#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__) */
How many times did I tell you that you should move this check inside the
if (ret < 0) block?
Finally some meta comments:
In your ping email you wrote about how frustrating this series has
become for you. Do you realise that it has become frustrating for
reviewers as well, and that that's the reason why you don't get quick
reviews any more?
Everyone assumes that, like every time, you still haven't addressed the
things that were commented on five versions ago (and hey, look above,
they were right!), and that reviewing it for the twelfth time would
simply be a waste of reviewers' time.
Reviewers getting tired of reviewing the same thing again is a problem
that any patch series that goes beyond v3 has. However, if you ignore
review comments repeatedly and/or you get close to v10, people will stop
thinking a more or less neutral "oh, another version of that series"
and start thinking a clearly negative "oh no, not again this crap".
That's where this series is, and not getting timely review (because the
maintainer is the only one who still _has_ to look at it and you've lost
everyone else) is a consequence of it.
Kevin
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-01-18 16:14 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-10 14:39 [Qemu-devel] ping [PATCH v11] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host Programmingkid
2015-12-11 22:00 ` [Qemu-devel] [Qemu-block] " Jeff Cody
2015-12-11 23:40 ` Programmingkid
2015-12-11 23:53 ` Eric Blake
2015-12-12 3:27 ` [Qemu-devel] [PATCH v12] " Programmingkid
2015-12-18 23:58 ` [Qemu-devel] ping " Programmingkid
2015-12-29 0:27 ` Programmingkid
2016-01-04 16:35 ` [Qemu-devel] [Qemu-block] " Max Reitz
2016-01-04 16:57 ` Programmingkid
2016-01-18 16:14 ` [Qemu-devel] " Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.