[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devic
From: |
Programmingkid |
Subject: |
Re: [Qemu-devel] ping [PATCH v14] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host |
Date: |
Wed, 2 Mar 2016 11:39:54 -0500 |
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 <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".
>
> 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.