Re: [Qemu-block] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Programmingkid]

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 <address@hidden>
>> 
>> ---
>> 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__) */.