Re: [Qemu-block] [PATCH v6] raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Programmingkid]

On Nov 24, 2015, at 9:38 AM, Kevin Wolf wrote:
> 
> 
>> +/* If using a physical device */
>> +if (strstart(filename, "/dev/", NULL)) {
>> +char bsdPath[MAXPATHLEN];
>> +
>> +/* If the physical device is a cdrom */
>> +if (strcmp(filename, "/dev/cdrom") == 0) {
> 
> The original code considered everything that starts in "/dev/cdrom", but
> this one only considers exact matches. Intentional?

Yes. CDROM's are handled differently from other kinds of devices. 

> 
> The outer strstart() check is redundant in this code as it is written.
> I'm not sure what you really wanted to do for the case that starts in
> "/dev/" but is different from "/dev/cdrom", but with this implementation
> nothing happens.
> 
>> +io_iterator_t mediaIterator;
>> +FindEjectableCDMedia();
>> +GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath), flags);
>> +if (bsdPath[0] == '\0') {
>> +printf("Error: failed to obtain bsd path for optical 
>> drive!\n");
> 
> If this is really an error, shouldn't we actually set errp and return
> from the function? And if it's not an error, being silent sounds right.

It is an error. But there is a chance that unmounting the device's volume from
the desktop might fix the problem, so letting the function continue to the 
helpful
error messages would be beneficial to the user. 

>> 
>> +#if defined(__APPLE__) && defined(__MACH__)
>> +/* if a physical device experienced an error while being opened */
>> +if (strncmp(filename, "/dev/", 5) == 0 && (cdromOK == false || ret != 
>> 0)) {
> 
> Using strstart() would probably be more consistent.

I would really prefer to stick with strncmp(). It is ANSI C and very well 
documented.
A search for strstart() did not turn up any documentation on it. 

> 
> I asked in v5 whether ret > 0 was possible (because otherwise the two
> 'if (ret < 0)' blocks could be merged) and you said it was. Now I
> reviewed raw_open_common() and I must say that I can't see how this
> function would ever return anything other than 0 or a negative errno.

It is possible the raw_open_common() function could be changed in the future
to produce positive error numbers. I think checking for anything that isn't zero
is the best thing to do.

[Qemu-block] [PATCH v6] raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

[Programmingkid]

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 

---
This patch depends on the GetBSDPath patch.

 block/raw-posix.c |   90 +---
 1 files changed, 64 insertions(+), 26 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index ccfec1c..28bce71 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
@@ -1975,7 +1974,7 @@ BlockDriver bdrv_file = {
 /* host device */
 
 #if defined(__APPLE__) && defined(__MACH__)
-static kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator );
+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 )
@@ -2033,7 +2032,34 @@ kern_return_t GetBSDPath(io_iterator_t mediaIterator, 
char *bsdPath,
 return kernResult;
 }
 
-#endif
+/* Sets up a real cdrom for use in QEMU */
+static bool setupCDROM(char *bsdPath)
+{
+int index, numOfTestPartitions = 2, fd;
+char testPartition[MAXPATHLEN];
+bool partitionFound = false;
+
+/* look for a working partition */
+for (index = 0; index < numOfTestPartitions; index++) {
+snprintf(testPartition, sizeof(testPartition), "%ss%d", bsdPath, 
index);
+fd = qemu_open(testPartition, O_RDONLY | O_BINARY | O_LARGEFILE);
+if (fd >= 0) {
+partitionFound = true;
+qemu_close(fd);
+break;
+}
+}
+
+/* if a working partition on the device was not found */
+if (partitionFound == false) {
+printf("Error: Failed to find a working partition on disc!\n");
+} else {
+DPRINTF("Using %s as optical disc\n", testPartition);
+pstrcpy(bsdPath, MAXPATHLEN, testPartition);
+}
+return partitionFound;
+}
+#endif /* defined(__APPLE__) && defined(__MACH__) */
 
 static int hdev_probe_device(const char *filename)
 {
@@ -2121,34 +2147,32 @@ static int hdev_open(BlockDriverState *bs, QDict 
*options, int flags,
 BDRVRawState *s = bs->opaque;
 Error *local_err = NULL;
 int ret;
+bool cdromOK = true;
 
 #if defined(__APPLE__) && defined(__MACH__)
 const char *filename = qdict_get_str(options, "filename");
-
-if (strstart(filename, "/dev/cdrom", NULL)) {
-kern_return_t kernResult;
-io_iterator_t mediaIterator;
-char bsdPath[ MAXPATHLEN ];
-int fd;
-
-kernResult = FindEjectableCDMedia(  );
-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';
+ 
+/* If using a physical device */
+if (strstart(filename, "/dev/", NULL)) {
+char bsdPath[MAXPATHLEN];
+
+/* If the physical device is a cdrom */
+if (strcmp(filename, "/dev/cdrom") == 0) {
+io_iterator_t mediaIterator;
+FindEjectableCDMedia();
+GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath), flags);
+if (bsdPath[0] == '\0') {
+printf("Error: failed to obtain bsd path for optical 
drive!\n");
 } else {
-qemu_close(fd);
+cdromOK = setupCDROM(bsdPath);
+filename = bsdPath;
+}
+
+if (mediaIterator) {
+IOObjectRelease(mediaIterator);
 }
-filename = bsdPath;
 qdict_put(options, "filename", qstring_from_str(filename));
 }
-
-if ( mediaIterator )
-IOObjectRelease( mediaIterator );
 }
 #endif
 
@@ -2159,7 +2183,21 @@ static int hdev_open(BlockDriverState *bs, QDict 
*options, int flags,
 if (local_err) {
 error_propagate(errp, local_err);
 }
-return ret;
+}
+
+#if defined(__APPLE__) && defined(__MACH__)
+/* if a physical device experienced an error while being opened */
+if (strncmp(filename, "/dev/", 5) == 0 && (cdromOK == false || ret != 0)) {
+printf("\nError: If device %s is mounted on the desktop, unmount it"
+" first before using it in QEMU.\n", filename);
+printf("Command to unmount device: diskutil unmountDisk %s\n",