Re: [Qemu-devel] [PATCH v8] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
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.
>
> Signed-off-by: John Arbuckle
>
> ---
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__) */
>
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 = 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.
> +fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
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 >= 0) {
> +partition_found = true;
> +qemu_close(fd);
> +break;
> +}
> +}
> +
> +/* 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");
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");
>
> +/* 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, unmount"
> + " it first before using it in QEMU.\n",
> file_name);
> +error_report("Command to unmount device: diskutil unmountDisk %s\n",
> +
> file_name);
> +error_report("Command to mount device: diskutil mountDisk %s\n",
> +
> file_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, QDict
> *options, int flags,
> int ret;
>
> #if defined(__APPLE__) && defined(__MACH__)
> -const char *filename = qdict_get_str(options, "filename");
> +const char *file_name = qdict_get_str(options, "filename");
No need to rename this variable.
> +
> +/* If a real optical drive was not found */
> +if (bsd_path[0] == '\0') {
> +error_setg(errp, "Error: failed to obtain bsd path for optical"
> + "
> drive!\n");
Again, weird indentation, redundant "Error: ", and no "!\n" at the end.
>
> +#if defined(__APPLE__) && defined(__MACH__)
> +/* if a physical device experienced an error while being opened */
> +if (strncmp(file_name, "/dev/", 5) == 0 && ret != 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 = hdev_is_sg(bs);
>
>
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v8] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
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
---
block/raw-posix.c | 98 +++--
1 files changed, 72 insertions(+), 26 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ccfec1c..d0190c1 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -42,9 +42,8 @@
#include
#include
#include
-//#include
#include
-#endif
+#endif /* (__APPLE__) && (__MACH__) */
#ifdef __sun__
#define _POSIX_PTHREAD_SEMANTICS 1
@@ -2033,7 +2032,36 @@ kern_return_t GetBSDPath(io_iterator_t mediaIterator,
char *bsdPath,
return kernResult;
}
-#endif
+/* Sets up a real cdrom for use in QEMU */
+static bool setup_cdrom(char *bsd_path, Error **errp)
+{
+int index, num_of_test_partitions = 2, fd;
+char test_partition[MAXPATHLEN];
+bool partition_found = false;
+
+/* 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);
+fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
+if (fd >= 0) {
+partition_found = true;
+qemu_close(fd);
+break;
+}
+}
+
+/* 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");
+} else {
+DPRINTF("Using %s as optical disc\n", test_partition);
+pstrcpy(bsd_path, MAXPATHLEN, test_partition);
+}
+return partition_found;
+}
+#endif /* defined(__APPLE__) && defined(__MACH__) */
static int hdev_probe_device(const char *filename)
{
@@ -2115,6 +2143,17 @@ static bool hdev_is_sg(BlockDriverState *bs)
return false;
}
+/* Prints directions on mounting and unmounting a device */
+static void printUnmountingDirections(const char *file_name)
+{
+error_report("Error: If device %s is mounted on the desktop, unmount"
+ " it first before using it in QEMU.\n",
file_name);
+error_report("Command to unmount device: diskutil unmountDisk %s\n",
+
file_name);
+error_report("Command to mount device: diskutil mountDisk %s\n",
+
file_name);
+}
+
static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -2123,32 +2162,32 @@ static int hdev_open(BlockDriverState *bs, QDict
*options, int flags,
int ret;
#if defined(__APPLE__) && defined(__MACH__)
-const char *filename = qdict_get_str(options, "filename");
+const char *file_name = qdict_get_str(options, "filename");
-if (strstart(filename, "/dev/cdrom", NULL)) {
-kern_return_t kernResult;
+/* If using a real cdrom */
+if (strcmp(file_name, "/dev/cdrom") == 0) {
+char bsd_path[MAXPATHLEN];
io_iterator_t mediaIterator;
-char bsdPath[ MAXPATHLEN ];
-int fd;
-
-kernResult = FindEjectableCDMedia( &mediaIterator );
-kernResult = GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath),
-flags);
-if ( bsdPath[ 0 ] != '\0' ) {
-strcat(bsdPath,"s0");
-/* some CDs don't have a partition 0 */
-fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
-if (fd < 0) {
-bsdPath[strlen(bsdPath)-1] = '1';
-} else {
-qemu_close(fd);
-}
-filename = bsdPath;
-qdict_put(options, "filename", qstring_from_str(filename));
+FindEjectableCDMedia(&mediaIterator);
+GetBSDPath(mediaIterator, bsd_path, sizeof(bsd_path), flags);
+if (mediaIterator) {
+IOObjectRelease(mediaIterator);
+}
+
+/* If a real optical drive was not found */
+if (bsd_path[0] == '\0') {
+error_setg(errp, "Error: failed to obtain bsd path for optical"
+ "
drive!\n");
+return -1;
}
-if ( mediaIterator )
-IOObjectRelease( mediaIterator );
+/* If finding a partition on the cdrom disc failed */
+if (setup_cdrom(bsd_path, errp) == false) {
+printUnmountingDirections(bsd_path);
+return -1;
+}
+file_name = bsd_path;
+qdict_put(options, "filename", qstring_from_str(file_name)
