[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v10] block/raw-posix.c: Make physic
From: |
Programmingkid |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v10] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host |
Date: |
Mon, 30 Nov 2015 11:51:48 -0500 |
On Nov 30, 2015, at 11:26 AM, Eric Blake wrote:
> On 11/27/2015 02:49 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.
>>
>> Signed-off-by: John Arbuckle <address@hidden>
>>
>> ---
>> Fixed some spacing issues.
>> Removed else condition in FindEjectableOpticalMedia.
>> Added continue statement to FindEjectableOpticalMedia.
>> Replaced printf() with error_report() in FindEjectableOpticalMedia.
>> Altered comment in FindEjectableOpticalMedia.
>> If the spacing in this patch looks off, try changing the font to something
>> that is mono-spaced.
>
> Patches are best read in monospaced fonts, anyways; it's better to make
> that part of your workflow, and assume that everyone else has already
> done likewise, than to advertise that you are only making life harder
> for yourself.
>
>>
>> block/raw-posix.c | 140 ++++++++++++++++++++++++++++++++++++++--------------
>> 1 files changed, 102 insertions(+), 38 deletions(-)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index ccfec1c..9e7de11 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -42,9 +42,9 @@
>> #include <IOKit/storage/IOMediaBSDClient.h>
>> #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
>> +#endif /* (__APPLE__) && (__MACH__) */
>>
>
> I have now mentioned in both v8 and v9 that this hunk should be its own
> patch (and is simple enough to cc qemu-trivial). Disregarding reviewers
> suggestions is not a good idea - it only serves to waste time (both
> yours and reviewers) and earn you black marks, such that it will be even
> less likely that anyone wants to review your patches in the first place.
> I'm trying to help you be a better contributor, but it feels like you
> are ignoring advice, and so my natural reaction is to ignore you.
I assure you that this change is *required* for my patch. Without it the patch
would
not even compile. I need a macro from IODVDMedia.h. If removing the IOCDTypes.h
is what is bothering you, it is a very small change that no one is going to
miss. That
header file was commented out but not removed for some reason.
I do thank you for your patients. I think it might be better if instead of
saying "this is wrong",
you talk about what should be done differently more.
>
>> #ifdef __sun__
>> #define _POSIX_PTHREAD_SEMANTICS 1
>> @@ -1975,32 +1975,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)
>
> No, your indentation is still wrong. I tried to point out on your v8
> that we don't right-justify to 80 columns, but rather left-justify to
> the point just after the (.
If you feel it is that important, I will do it. I just thought it was easier to
read when your
eye is already in the area. There is less time spend finding the next argument
that way.
>
>> + 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.\n",
>
> No. Don't use trailing '.' or trailing '\n' in error_report() calls.
> I've already mentioned this, and feel like I'm becoming a broken record.
> When you disregard my review comments, I become disinclined to review
> your patches any further.
I don't remember hearing about not using \n in the error_report() call, but I
will
fix this in the next patch.
>
>> +
>> matching_array[index]);
>
> Indentation is still wrong.
Will left justify with the left parenthesis.
>
>> + continue;
>> + }
>> + CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
>> +
>> kCFBooleanTrue);
>> + kernResult = IOServiceGetMatchingServices(masterPort,
>> classesToMatch,
>> +
>> mediaIterator);
>> + if (kernResult != KERN_SUCCESS) {
>> + error_report("Note: IOServiceGetMatchingServices returned %d\n",
>> +
>> kernResult);
>
> No trailing \n in error_report(), indentation is wrong.
Ok.
>
>> + }
>>
>> + /* 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]);
>
> Spaces around binary '+', and indentation is wrong.
Ok. Will add spaces and left justify.
>
>
>> + /* 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);
>
> Indentation is wrong.
Ok. Will left justify also.
>
>
>> + /* if a working partition on the device was not found */
>> + if (partition_found == false) {
>> + error_setg(errp, "Error: Failed to find a working partition on "
>> +
>> "disc!\n");
>
> Indentation is wrong, no trailing '!' or '\n' in error_setg().
Keeping spaces around the messages made them easier to read. But
I'm flexible. Will remove the ! and \n.
>
> I'm so bothered by the fact that I already pointed this out in v9 and
> you still didn't fix it for v10 that I am not even paying attention to
> actual code, and just looking at style violations. You have effectively
> lost me as a valid reviewer on this patch. I don't like feeling like
> this, as I try hard to be welcoming to new contributors, but in the open
> source world, you have to return the favor by learning from the advice
> you are given, rather than repeating the same mistakes.
Please forgive me of my sins. I am just another human being trying to help
improve QEMU. Practicing a little more patients and tolerance might help
improve your life.