* [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
@ 2016-02-29 15:17 Programmingkid
2016-03-01 15:16 ` Kevin Wolf
0 siblings, 1 reply; 7+ messages in thread
From: Programmingkid @ 2016-02-29 15:17 UTC (permalink / raw)
To: Kevin Wolf, qemu-devel qemu-devel, Qemu-block
I do think this patch is ready to be added to QEMU. I have listened to what you said and implemented your changes.
https://patchwork.ozlabs.org/patch/579325/
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>
---
Changed filename variable to const char * type.
Removed snprintf call for filename variable.
filename is set to bsd_path if using a physical device that isn't a DVD or CD.
block/raw-posix.c | 167 ++++++++++++++++++++++++++++++++++++++++-------------
1 files changed, 127 insertions(+), 40 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6df3067..48dc5a8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -44,6 +44,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
@@ -1963,33 +1964,47 @@ 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);
+ continue;
+ }
- 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,
@@ -2021,7 +2036,46 @@ 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;
+}
+
+/* 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);
+}
+
+#endif /* defined(__APPLE__) && defined(__MACH__) */
static int hdev_probe_device(const char *filename)
{
@@ -2112,33 +2166,57 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
#if defined(__APPLE__) && defined(__MACH__)
const char *filename = qdict_get_str(options, "filename");
+ char bsd_path[MAXPATHLEN] = "";
+ bool error_occurred = false;
+
+ /* If using a real cdrom */
+ if (strcmp(filename, "/dev/cdrom") == 0) {
+ char *mediaType = NULL;
+ kern_return_t ret_val;
+ io_iterator_t mediaIterator = 0;
+
+ mediaType = FindEjectableOpticalMedia(&mediaIterator);
+ if (mediaType == NULL) {
+ error_setg(errp, "Please make sure your CD/DVD is in the optical"
+ " drive");
+ error_occurred = true;
+ goto hdev_open_Mac_error;
+ }
- 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));
+ ret_val = GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags);
+ if (ret_val != KERN_SUCCESS) {
+ error_setg(errp, "Could not get BSD path for optical drive");
+ error_occurred = true;
+ goto hdev_open_Mac_error;
+ }
+
+ /* If a real optical drive was not found */
+ if (bsd_path[0] == '\0') {
+ error_setg(errp, "Failed to obtain bsd path for optical drive");
+ error_occurred = true;
+ goto hdev_open_Mac_error;
}
- if ( mediaIterator )
- IOObjectRelease( mediaIterator );
+ /* If using a cdrom disc and finding a partition on the disc failed */
+ if (strncmp(mediaType, kIOCDMediaClass, 9) == 0 &&
+ setup_cdrom(bsd_path, errp) == false) {
+ print_unmounting_directions(bsd_path);
+ error_occurred = true;
+ goto hdev_open_Mac_error;
+ }
+
+ qdict_put(options, "filename", qstring_from_str(bsd_path));
+
+hdev_open_Mac_error:
+ g_free(mediaType);
+ if (mediaIterator) {
+ IOObjectRelease(mediaIterator);
+ }
+ if (error_occurred) {
+ return -1;
+ }
}
-#endif
+#endif /* defined(__APPLE__) && defined(__MACH__) */
s->type = FTYPE_FILE;
@@ -2147,7 +2225,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 (*bsd_path) {
+ filename = bsd_path;
+ }
+ /* if a physical device experienced an error while being opened */
+ if (strncmp(filename, "/dev/", 5) == 0) {
+ print_unmounting_directions(filename);
+ return -1;
+ }
+#endif /* defined(__APPLE__) && defined(__MACH__) */
}
/* Since this does ioctl the device must be already opened */
--
1.7.5.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2016-02-29 15:17 [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host Programmingkid
@ 2016-03-01 15:16 ` Kevin Wolf
2016-03-01 16:25 ` [Qemu-devel] [Qemu-block] " Jeff Cody
2016-03-02 3:32 ` [Qemu-devel] " Programmingkid
0 siblings, 2 replies; 7+ messages in thread
From: Kevin Wolf @ 2016-03-01 15:16 UTC (permalink / raw)
To: Programmingkid; +Cc: qemu-devel qemu-devel, Qemu-block
Am 29.02.2016 um 16:17 hat Programmingkid geschrieben:
> I do think this patch is ready to be added to QEMU. I have listened to what you said and implemented your changes.
>
> https://patchwork.ozlabs.org/patch/579325/
>
> 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>
>
> ---
> Changed filename variable to const char * type.
> Removed snprintf call for filename variable.
> filename is set to bsd_path if using a physical device that isn't a DVD or CD.
> @@ -2112,33 +2166,57 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
>
> #if defined(__APPLE__) && defined(__MACH__)
> const char *filename = qdict_get_str(options, "filename");
> + char bsd_path[MAXPATHLEN] = "";
> + bool error_occurred = false;
> +
This line adds trailing whitespace.
> @@ -2147,7 +2225,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 (*bsd_path) {
> + filename = bsd_path;
> + }
> + /* if a physical device experienced an error while being opened */
> + if (strncmp(filename, "/dev/", 5) == 0) {
> + print_unmounting_directions(filename);
> + return -1;
Please use a negative errno number instead of -1.
But more importantly: What happened with the return that you removed
above? Even in the non-Apple case, we don't return an error now, but
continue in this function. This looks certainly wrong. Did you intend to
move it to below the #ifdef block?
Kevin
> + }
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
> }
>
> /* Since this does ioctl the device must be already opened */
> --
> 1.7.5.4
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [Qemu-block] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2016-03-01 15:16 ` Kevin Wolf
@ 2016-03-01 16:25 ` Jeff Cody
2016-03-02 3:32 ` [Qemu-devel] " Programmingkid
1 sibling, 0 replies; 7+ messages in thread
From: Jeff Cody @ 2016-03-01 16:25 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Programmingkid, qemu-devel qemu-devel, Qemu-block
On Tue, Mar 01, 2016 at 04:16:39PM +0100, Kevin Wolf wrote:
> Am 29.02.2016 um 16:17 hat Programmingkid geschrieben:
> > I do think this patch is ready to be added to QEMU. I have listened to what you said and implemented your changes.
> >
> > https://patchwork.ozlabs.org/patch/579325/
> >
> > 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>
> >
> > ---
> > Changed filename variable to const char * type.
> > Removed snprintf call for filename variable.
> > filename is set to bsd_path if using a physical device that isn't a DVD or CD.
>
> > @@ -2112,33 +2166,57 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
> >
> > #if defined(__APPLE__) && defined(__MACH__)
> > const char *filename = qdict_get_str(options, "filename");
> > + char bsd_path[MAXPATHLEN] = "";
> > + bool error_occurred = false;
> > +
>
> This line adds trailing whitespace.
>
> > @@ -2147,7 +2225,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 (*bsd_path) {
> > + filename = bsd_path;
> > + }
> > + /* if a physical device experienced an error while being opened */
> > + if (strncmp(filename, "/dev/", 5) == 0) {
> > + print_unmounting_directions(filename);
> > + return -1;
>
> Please use a negative errno number instead of -1.
>
> But more importantly: What happened with the return that you removed
> above? Even in the non-Apple case, we don't return an error now, but
> continue in this function. This looks certainly wrong. Did you intend to
> move it to below the #ifdef block?
>
> Kevin
>
> > + }
> > +#endif /* defined(__APPLE__) && defined(__MACH__) */
> > }
> >
> > /* Since this does ioctl the device must be already opened */
> > --
> > 1.7.5.4
> >
> >
>
In addition to the above, the patch is still using quoted-printable
encoding. With Mutt at least, this makes applying patches cumbersome.
Whether or not this is an issue for Kevin's workflow, I don't know.
But if you are re-spinning a new patch, I recommend taking care of
that (git send-email will do the right thing).
Jeff
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2016-03-01 15:16 ` Kevin Wolf
2016-03-01 16:25 ` [Qemu-devel] [Qemu-block] " Jeff Cody
@ 2016-03-02 3:32 ` Programmingkid
2016-03-02 9:02 ` Kevin Wolf
1 sibling, 1 reply; 7+ messages in thread
From: Programmingkid @ 2016-03-02 3:32 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel qemu-devel, Qemu-block
On Mar 1, 2016, at 10:16 AM, Kevin Wolf wrote:
> Am 29.02.2016 um 16:17 hat Programmingkid geschrieben:
>> I do think this patch is ready to be added to QEMU. I have listened to what you said and implemented your changes.
>>
>> https://patchwork.ozlabs.org/patch/579325/
>>
>> 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>
>>
>> ---
>> Changed filename variable to const char * type.
>> Removed snprintf call for filename variable.
>> filename is set to bsd_path if using a physical device that isn't a DVD or CD.
>
>> @@ -2112,33 +2166,57 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
>>
>> #if defined(__APPLE__) && defined(__MACH__)
>> const char *filename = qdict_get_str(options, "filename");
>> + char bsd_path[MAXPATHLEN] = "";
>> + bool error_occurred = false;
>> +
>
> This line adds trailing whitespace.
>
>> @@ -2147,7 +2225,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 (*bsd_path) {
>> + filename = bsd_path;
>> + }
>> + /* if a physical device experienced an error while being opened */
>> + if (strncmp(filename, "/dev/", 5) == 0) {
>> + print_unmounting_directions(filename);
>> + return -1;
>
> Please use a negative errno number instead of -1.
Is this ok:
return -EPERM;
According to http://man7.org/linux/man-pages/man3/errno.3.html, it means "operation not permitted".
Did you want this -1 changed also?
+hdev_open_Mac_error:
+ g_free(mediaType);
+ if (mediaIterator) {
+ IOObjectRelease(mediaIterator);
+ }
+ if (error_occurred) {
+ return -1;
+ }
>
> But more importantly: What happened with the return that you removed
> above? Even in the non-Apple case, we don't return an error now, but
> continue in this function. This looks certainly wrong. Did you intend to
> move it to below the #ifdef block?
Good catch. I will place it right below the #endif /* defined(__APPLE__) && defined(__MACH__) */.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2016-03-02 3:32 ` [Qemu-devel] " Programmingkid
@ 2016-03-02 9:02 ` Kevin Wolf
2016-03-02 16:39 ` Programmingkid
0 siblings, 1 reply; 7+ messages in thread
From: Kevin Wolf @ 2016-03-02 9:02 UTC (permalink / raw)
To: Programmingkid; +Cc: qemu-devel qemu-devel, Qemu-block
Am 02.03.2016 um 04:32 hat Programmingkid geschrieben:
>
> On Mar 1, 2016, at 10:16 AM, Kevin Wolf wrote:
>
> > Am 29.02.2016 um 16:17 hat Programmingkid geschrieben:
> >> I do think this patch is ready to be added to QEMU. I have listened to what you said and implemented your changes.
> >>
> >> https://patchwork.ozlabs.org/patch/579325/
> >>
> >> 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>
> >>
> >> ---
> >> Changed filename variable to const char * type.
> >> Removed snprintf call for filename variable.
> >> filename is set to bsd_path if using a physical device that isn't a DVD or CD.
> >
> >> @@ -2112,33 +2166,57 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
> >>
> >> #if defined(__APPLE__) && defined(__MACH__)
> >> const char *filename = qdict_get_str(options, "filename");
> >> + char bsd_path[MAXPATHLEN] = "";
> >> + bool error_occurred = false;
> >> +
> >
> > This line adds trailing whitespace.
> >
> >> @@ -2147,7 +2225,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 (*bsd_path) {
> >> + filename = bsd_path;
> >> + }
> >> + /* if a physical device experienced an error while being opened */
> >> + if (strncmp(filename, "/dev/", 5) == 0) {
> >> + print_unmounting_directions(filename);
> >> + return -1;
> >
> > Please use a negative errno number instead of -1.
>
> Is this ok:
> return -EPERM;
>
> According to http://man7.org/linux/man-pages/man3/errno.3.html, it means "operation not permitted".
Well, to be honest I don't understand why there is a different error
code here to begin with. Maybe when you add the "return ret" back after
the #endif you can just leave out this line and return the real error
code this way.
If for some reason (that I fail to understand) ret doesn't contain an
appropriate error code in this case, though, you can return a constant.
If it's related to permissions, -EPERM is okay, otherwise it's probably
not. I don't see a connection between paths starting with /dev/ and
there being a permission problem.
> Did you want this -1 changed also?
> +hdev_open_Mac_error:
> + g_free(mediaType);
> + if (mediaIterator) {
> + IOObjectRelease(mediaIterator);
> + }
> + if (error_occurred) {
> + return -1;
> + }
Yes, please, I missed that one. We don't have a valid ret here, so maybe
-ENOENT would be the closest one?
> > But more importantly: What happened with the return that you removed
> > above? Even in the non-Apple case, we don't return an error now, but
> > continue in this function. This looks certainly wrong. Did you intend to
> > move it to below the #ifdef block?
>
> Good catch. I will place it right below the #endif /* defined(__APPLE__) && defined(__MACH__) */.
Thanks.
Kevin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2016-03-02 9:02 ` Kevin Wolf
@ 2016-03-02 16:39 ` Programmingkid
2016-03-02 16:49 ` Kevin Wolf
0 siblings, 1 reply; 7+ messages in thread
From: Programmingkid @ 2016-03-02 16:39 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel qemu-devel, Qemu-block
On Mar 2, 2016, at 4:02 AM, Kevin Wolf wrote:
> Am 02.03.2016 um 04:32 hat Programmingkid geschrieben:
>>
>> On Mar 1, 2016, at 10:16 AM, Kevin Wolf wrote:
>>
>>> Am 29.02.2016 um 16:17 hat Programmingkid geschrieben:
>>>> I do think this patch is ready to be added to QEMU. I have listened to what you said and implemented your changes.
>>>>
>>>> https://patchwork.ozlabs.org/patch/579325/
>>>>
>>>> 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>
>>>>
>>>> ---
>>>> Changed filename variable to const char * type.
>>>> Removed snprintf call for filename variable.
>>>> filename is set to bsd_path if using a physical device that isn't a DVD or CD.
>>>
>>>> @@ -2112,33 +2166,57 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
>>>>
>>>> #if defined(__APPLE__) && defined(__MACH__)
>>>> const char *filename = qdict_get_str(options, "filename");
>>>> + char bsd_path[MAXPATHLEN] = "";
>>>> + bool error_occurred = false;
>>>> +
>>>
>>> This line adds trailing whitespace.
>>>
>>>> @@ -2147,7 +2225,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 (*bsd_path) {
>>>> + filename = bsd_path;
>>>> + }
>>>> + /* if a physical device experienced an error while being opened */
>>>> + if (strncmp(filename, "/dev/", 5) == 0) {
>>>> + print_unmounting_directions(filename);
>>>> + return -1;
>>>
>>> Please use a negative errno number instead of -1.
>>
>> Is this ok:
>> return -EPERM;
>>
>> According to http://man7.org/linux/man-pages/man3/errno.3.html, it means "operation not permitted".
>
> Well, to be honest I don't understand why there is a different error
> code here to begin with. Maybe when you add the "return ret" back after
> the #endif you can just leave out this line and return the real error
> code this way.
I like this idea more.
>
> If for some reason (that I fail to understand) ret doesn't contain an
> appropriate error code in this case, though, you can return a constant.
> If it's related to permissions, -EPERM is okay, otherwise it's probably
> not. I don't see a connection between paths starting with /dev/ and
> there being a permission problem.
I have placed "return ret;" right after the #endif and removed the "return -1" in the if condition because it is now redundant. Does this look right:
if (ret < 0) {
if (local_err) {
error_propagate(errp, local_err);
}
#if defined(__APPLE__) && defined(__MACH__)
if (*bsd_path) {
filename = bsd_path;
}
/* if a physical device experienced an error while being opened */
if (strncmp(filename, "/dev/", 5) == 0) {
print_unmounting_directions(filename);
}
#endif /* defined(__APPLE__) && defined(__MACH__) */
return ret;
}
>
>> Did you want this -1 changed also?
>> +hdev_open_Mac_error:
>> + g_free(mediaType);
>> + if (mediaIterator) {
>> + IOObjectRelease(mediaIterator);
>> + }
>> + if (error_occurred) {
>> + return -1;
>> + }
>
> Yes, please, I missed that one. We don't have a valid ret here, so maybe
> -ENOENT would be the closest one?
Ok.
>
>>> But more importantly: What happened with the return that you removed
>>> above? Even in the non-Apple case, we don't return an error now, but
>>> continue in this function. This looks certainly wrong. Did you intend to
>>> move it to below the #ifdef block?
>>
>> Good catch. I will place it right below the #endif /* defined(__APPLE__) && defined(__MACH__) */.
>
> Thanks.
>
> Kevin
Thank you.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
2016-03-02 16:39 ` Programmingkid
@ 2016-03-02 16:49 ` Kevin Wolf
0 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2016-03-02 16:49 UTC (permalink / raw)
To: Programmingkid; +Cc: qemu-devel qemu-devel, Qemu-block
Am 02.03.2016 um 17:39 hat Programmingkid geschrieben:
>
> On Mar 2, 2016, at 4:02 AM, Kevin Wolf wrote:
>
> > Am 02.03.2016 um 04:32 hat Programmingkid geschrieben:
> >>
> >> On Mar 1, 2016, at 10:16 AM, Kevin Wolf wrote:
> >>
> >>> Am 29.02.2016 um 16:17 hat Programmingkid geschrieben:
> >>>> I do think this patch is ready to be added to QEMU. I have listened to what you said and implemented your changes.
> >>>>
> >>>> https://patchwork.ozlabs.org/patch/579325/
> >>>>
> >>>> 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>
> >>>>
> >>>> ---
> >>>> Changed filename variable to const char * type.
> >>>> Removed snprintf call for filename variable.
> >>>> filename is set to bsd_path if using a physical device that isn't a DVD or CD.
> >>>
> >>>> @@ -2112,33 +2166,57 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
> >>>>
> >>>> #if defined(__APPLE__) && defined(__MACH__)
> >>>> const char *filename = qdict_get_str(options, "filename");
> >>>> + char bsd_path[MAXPATHLEN] = "";
> >>>> + bool error_occurred = false;
> >>>> +
> >>>
> >>> This line adds trailing whitespace.
> >>>
> >>>> @@ -2147,7 +2225,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 (*bsd_path) {
> >>>> + filename = bsd_path;
> >>>> + }
> >>>> + /* if a physical device experienced an error while being opened */
> >>>> + if (strncmp(filename, "/dev/", 5) == 0) {
> >>>> + print_unmounting_directions(filename);
> >>>> + return -1;
> >>>
> >>> Please use a negative errno number instead of -1.
> >>
> >> Is this ok:
> >> return -EPERM;
> >>
> >> According to http://man7.org/linux/man-pages/man3/errno.3.html, it means "operation not permitted".
> >
> > Well, to be honest I don't understand why there is a different error
> > code here to begin with. Maybe when you add the "return ret" back after
> > the #endif you can just leave out this line and return the real error
> > code this way.
>
> I like this idea more.
>
> >
> > If for some reason (that I fail to understand) ret doesn't contain an
> > appropriate error code in this case, though, you can return a constant.
> > If it's related to permissions, -EPERM is okay, otherwise it's probably
> > not. I don't see a connection between paths starting with /dev/ and
> > there being a permission problem.
>
> I have placed "return ret;" right after the #endif and removed the "return -1" in the if condition because it is now redundant. Does this look right:
>
> if (ret < 0) {
> if (local_err) {
> error_propagate(errp, local_err);
> }
> #if defined(__APPLE__) && defined(__MACH__)
> if (*bsd_path) {
> filename = bsd_path;
> }
> /* if a physical device experienced an error while being opened */
> if (strncmp(filename, "/dev/", 5) == 0) {
> print_unmounting_directions(filename);
> }
> #endif /* defined(__APPLE__) && defined(__MACH__) */
> return ret;
> }
Looks right to me.
Kevin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-03-02 16:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-29 15:17 [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host Programmingkid
2016-03-01 15:16 ` Kevin Wolf
2016-03-01 16:25 ` [Qemu-devel] [Qemu-block] " Jeff Cody
2016-03-02 3:32 ` [Qemu-devel] " Programmingkid
2016-03-02 9:02 ` Kevin Wolf
2016-03-02 16:39 ` Programmingkid
2016-03-02 16:49 ` Kevin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).