From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45224) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3RgA-0006nl-Kb for qemu-devel@nongnu.org; Mon, 30 Nov 2015 11:51:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3Rg7-0001V9-Ax for qemu-devel@nongnu.org; Mon, 30 Nov 2015 11:51:54 -0500 Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: text/plain; charset=us-ascii From: Programmingkid In-Reply-To: <565C78C5.1010203@redhat.com> Date: Mon, 30 Nov 2015 11:51:48 -0500 Content-Transfer-Encoding: quoted-printable Message-Id: References: <565C78C5.1010203@redhat.com> Subject: Re: [Qemu-devel] [PATCH v10] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , qemu-devel qemu-devel , Qemu-block 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. >>=20 >> Signed-off-by: John Arbuckle >>=20 >> --- >> Fixed some spacing issues.=20 >> 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. >=20 > 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. >=20 >>=20 >> block/raw-posix.c | 140 = ++++++++++++++++++++++++++++++++++++++-------------- >> 1 files changed, 102 insertions(+), 38 deletions(-) >>=20 >> 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 >> #include >> #include >> -//#include >> +#include >> #include >> -#endif >> +#endif /* (__APPLE__) && (__MACH__) */ >>=20 >=20 > 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.=20 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. >=20 >> #ifdef __sun__ >> #define _POSIX_PTHREAD_SEMANTICS 1 >> @@ -1975,32 +1975,46 @@ BlockDriver bdrv_file =3D { >> /* host device */ >>=20 >> #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) >=20 > 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. >=20 >> + int index; >> + for (index =3D 0; index < ARRAY_SIZE(matching_array); index++) { >> + classesToMatch =3D IOServiceMatching(matching_array[index]); >> + if (classesToMatch =3D=3D NULL) { >> + error_report("IOServiceMatching returned NULL for = %s.\n", >=20 > 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. >=20 >> + = matching_array[index]); >=20 > Indentation is still wrong. Will left justify with the left parenthesis.=20 >=20 >> + continue; >> + } >> + CFDictionarySetValue(classesToMatch, = CFSTR(kIOMediaEjectableKey), >> + = kCFBooleanTrue); >> + kernResult =3D IOServiceGetMatchingServices(masterPort, = classesToMatch, >> + = mediaIterator); >> + if (kernResult !=3D KERN_SUCCESS) { >> + error_report("Note: IOServiceGetMatchingServices = returned %d\n", >> + = kernResult); >=20 > No trailing \n in error_report(), indentation is wrong. Ok.=20 >=20 >> + } >>=20 >> + /* If a match was found, leave the loop */ >> + if (*mediaIterator !=3D 0) { >> + DPRINTF("Matching using %s\n", matching_array[index]); >> + snprintf(mediaType, strlen(matching_array[index])+1, = "%s", >> + = matching_array[index]); >=20 > Spaces around binary '+', and indentation is wrong. Ok. Will add spaces and left justify. >=20 >=20 >> + /* look for a working partition */ >> + for (index =3D 0; index < num_of_test_partitions; index++) { >> + snprintf(test_partition, sizeof(test_partition), "%ss%d", = bsd_path, >> + = index); >=20 > Indentation is wrong. Ok. Will left justify also. >=20 >=20 >> + /* if a working partition on the device was not found */ >> + if (partition_found =3D=3D false) { >> + error_setg(errp, "Error: Failed to find a working partition = on " >> + = "disc!\n"); >=20 > 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.=20 >=20 > 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.=