From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35199) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3RHv-0002ku-Hc for qemu-devel@nongnu.org; Mon, 30 Nov 2015 11:26:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3RHu-0000wJ-0O for qemu-devel@nongnu.org; Mon, 30 Nov 2015 11:26:51 -0500 References: From: Eric Blake Message-ID: <565C78C5.1010203@redhat.com> Date: Mon, 30 Nov 2015 09:26:45 -0700 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="wh6EXAgHHHH7PxaFtRuI4PSnFoPAcPdig" 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: Programmingkid , Kevin Wolf Cc: qemu-devel qemu-devel , Qemu-block This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --wh6EXAgHHHH7PxaFtRuI4PSnFoPAcPdig Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 someth= ing > 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. >=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 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. > #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 *mediaIterato= r ); > static kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsd= Path, > CFIndex maxPathSize, int flags); > -kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator ) > +static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIte= rator, > + 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 (. > + 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", 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. > + matching_arra= y[index]); Indentation is still wrong. > + continue; > + } > + CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKe= y), > + kCFBoo= leanTrue); > + kernResult =3D IOServiceGetMatchingServices(masterPort, classe= sToMatch, > + media= Iterator); > + if (kernResult !=3D KERN_SUCCESS) { > + error_report("Note: IOServiceGetMatchingServices returned = %d\n", > + ke= rnResult); No trailing \n in error_report(), indentation is wrong. > + } > =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_arra= y[index]); Spaces around binary '+', and indentation is wrong. > + /* 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); Indentation is wrong. > + /* 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"); Indentation is wrong, no trailing '!' or '\n' in error_setg(). 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --wh6EXAgHHHH7PxaFtRuI4PSnFoPAcPdig Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWXHjFAAoJEKeha0olJ0NqYIEH/ij9maHVaWUlxEjs0k5VUQWr r5qy5XusfDquuE0APo+28VaJJePrWCAFUqG62fhPSd4cNAw0+xPR7rsmJr4wA0KR SuJW5aM7JEftw2R57e5NjaF4vfp2umMQzXzut+/aUNLPHkJcvBV+au+FfRPcYl7O i3dF0Js+d3AIzPF0JwGPhT79KyEXJLz+XYX51ACQ3xKA7xuyd+GfldC6qPvSLYXt DbCVOsMFFpKQTk09q1LkUTlfsp5hwtH0yrWs3huQZVGzKS70BhFAqeFY0hmlnUrx 9FwIgdMyLvVb6BJ7Wlo9vtNZagwRMeP05XBNH9Nii5NHG6zg98Wp7ljGhBZNbQg= =qvie -----END PGP SIGNATURE----- --wh6EXAgHHHH7PxaFtRuI4PSnFoPAcPdig--