From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38545) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1kyX-0002VU-0B for qemu-devel@nongnu.org; Wed, 25 Nov 2015 20:03:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1kyV-0007Dl-PO for qemu-devel@nongnu.org; Wed, 25 Nov 2015 20:03:52 -0500 References: <0CCF4621-0581-41FD-A207-9FE5E1EDC58D@gmail.com> From: Eric Blake Message-ID: <56565A6B.2040309@redhat.com> Date: Wed, 25 Nov 2015 18:03:39 -0700 MIME-Version: 1.0 In-Reply-To: <0CCF4621-0581-41FD-A207-9FE5E1EDC58D@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="itOF0mwxe82s7sBTX4wIO5uw6vC1437PM" Subject: Re: [Qemu-devel] [PATCH v8] 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) --itOF0mwxe82s7sBTX4wIO5uw6vC1437PM Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/25/2015 05:24 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 > --- Right here (between the --- and diffstat) it's nice to post a changelog of how v8 differs from v7, to help earlier reviewers focus on the improvements. > block/raw-posix.c | 98 +++++++++++++++++++++++++++++++++++++++------= -------- > 1 files changed, 72 insertions(+), 26 deletions(-) > +++ b/block/raw-posix.c > @@ -42,9 +42,8 @@ > #include > #include > #include > -//#include > #include > -#endif > +#endif /* (__APPLE__) && (__MACH__) */ > =20 This hunk looks to be an unrelated cleanup; you might want to just propose it separately through the qemu-trivial queue (but don't forget that even trivial patches must cc qemu-devel). > + > + /* 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); Unusual indentation. More typical would be: snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path, index); with the second line flush to the character after the ( of the first line= =2E > + fd =3D qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGE= FILE); Isn't qemu_open() supposed to provide O_LARGEFILE for ALL users automatically? (That is, why would we ever _want_ to handle a file using only 32-bit off_t?) But that's a separate issue; it looks like you are copy-and-pasting from existing use of this idiom already in raw-posix.c. > + if (fd >=3D 0) { > + partition_found =3D true; > + qemu_close(fd); > + break; > + } > + } > + > + /* 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"); Several violations of convention. error_setg() does not need a redundant "Error: " prefix, should not end in '!' (we aren't shouting), and should not end in newline. And with those fixes, you won't even need the weird indentation. error_setg(errp, "failed to find a working partition on disk"); > =20 > +/* Prints directions on mounting and unmounting a device */ > +static void printUnmountingDirections(const char *file_name) Elsewhere, we use 'function_name', not 'functionName'. > +{ > + error_report("Error: If device %s is mounted on the desktop, unmou= nt" > + " it first before using it in QEMU.\n", f= ile_name); > + error_report("Command to unmount device: diskutil unmountDisk %s\n= ", > + f= ile_name); > + error_report("Command to mount device: diskutil mountDisk %s\n", > + f= ile_name); Again, weird indentation. And don't use \n at the end of error_report(). > @@ -2123,32 +2162,32 @@ static int hdev_open(BlockDriverState *bs, QDic= t *options, int flags, > int ret; > =20 > #if defined(__APPLE__) && defined(__MACH__) > - const char *filename =3D qdict_get_str(options, "filename"); > + const char *file_name =3D qdict_get_str(options, "filename"); No need to rename this variable. > + > + /* If a real optical drive was not found */ > + if (bsd_path[0] =3D=3D '\0') { > + error_setg(errp, "Error: failed to obtain bsd path for opt= ical" > + " d= rive!\n"); Again, weird indentation, redundant "Error: ", and no "!\n" at the end. > =20 > +#if defined(__APPLE__) && defined(__MACH__) > + /* if a physical device experienced an error while being opened */= > + if (strncmp(file_name, "/dev/", 5) =3D=3D 0 && ret !=3D 0) { > + printUnmountingDirections(file_name); Is this advice appropriate to ALL things under /dev/, or just cdroms? > + return -1; > + } > +#endif /* defined(__APPLE__) && defined(__MACH__) */ > + > /* Since this does ioctl the device must be already opened */ > bs->sg =3D hdev_is_sg(bs); > =20 >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --itOF0mwxe82s7sBTX4wIO5uw6vC1437PM 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/ iQEcBAEBCAAGBQJWVlprAAoJEKeha0olJ0NqzbgH/Rg1mzyD/i9btAlqwScFV7DL G3Kcq7CcztQcljS7t0qzdesv0KvDkE4BWhLgeWUZerAClhbOlmFMWLapzQ0Bivmf Zv96wdpL4zLCaOD0jozwSLVnIcsPqtlPMhnDmYGNUqLvjDNtqKoAbIKvk+ixDgro WpTJzLG/qh4oVFtUFS6WFCVbz1Mf65uMHdhiOahF6tGTk1K09UD2B+CiHrNXvvKx dyhNg72a6f8Ii25NSiPhUVexcQPNR11i2Ym1vnCfvnxWBQUMqiX3MjYT4mmF7G8b q5FqAvhVrU5d+FeStpFOWjIIApO0qrBuYSWQA6Ff4DzBpNU4Eayxg6gUtgzN+Is= =Qi14 -----END PGP SIGNATURE----- --itOF0mwxe82s7sBTX4wIO5uw6vC1437PM--