diff mbox

[v3] raw-posix.c: Make physical devices usable in QEMU under Mac OS X host

Message ID 4E49353C-F0D9-43CA-B4FE-BB5E85FCEEDC@gmail.com
State New
Headers show

Commit Message

Programmingkid July 18, 2015, 12:19 a.m. UTC 
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 <programmingkidx@gmail.com>

---
Replaced strncpy with pstrcpy.
Eliminated the setupDevice function.
Placed helpful error message after call to raw_open_common().

 block/raw-posix.c |  100 ++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 72 insertions(+), 28 deletions(-)

Comments

Stefan Hajnoczi July 24, 2015, 3 p.m. UTC | #1 
On Fri, Jul 17, 2015 at 08:19:16PM -0400, Programmingkid wrote:
> @@ -2014,7 +2015,9 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex ma
>          if ( bsdPathAsCFString ) {
>              size_t devPathLength;
>              strcpy( bsdPath, _PATH_DEV );
> -            strcat( bsdPath, "r" );
> +            if (flags & BDRV_O_NOCACHE) {
> +                strcat(bsdPath, "r");
> +            }
>              devPathLength = strlen( bsdPath );
>              if ( CFStringGetCString( bsdPathAsCFString, bsdPath + devPathLength, maxPathSize - devPathLength, kCFStringEncodingASCII ) ) {
>                  kernResult = KERN_SUCCESS;

This hunk should be a separate patch.  It fixes raw-posix alignment
probing by only using the raw char device (which has alignment
constraints) if BDRV_O_NOCACHE was given.

> @@ -2027,7 +2030,35 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex ma
>      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++) {
> +        pstrcpy(testPartition, strlen(bsdPath)+1, bsdPath);

The safe way to use pstrcpy is:

char dest[LEN];
pstrcpy(dest, sizeof(dest), src);

Use the destination buffer size since that's what needs to be checked to
prevent buffer overflow.

Using the source buffer size could cause an overflow if the source
buffer is larger than the destination buffer.  Even if that's not the
case today, it's bad practice because it could lead to bugs if code is
modified.

> +        snprintf(testPartition, MAXPATHLEN, "%ss%d", testPartition, index);

Using the same buffer as the destination and a format string argument is
questionable.  I wouldn't be surprised if some snprintf()
implementations produce garbage when you make them read from the same
buffer they are writing to.

Please replace pstrcpy() and snprintf() with a single call:

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, strlen(testPartition)+1, testPartition);

Please use MAXPATHLEN instead of strlen(testPartition)+1.
Programmingkid July 24, 2015, 3:37 p.m. UTC | #2 
On Jul 24, 2015, at 11:00 AM, Stefan Hajnoczi wrote:

> On Fri, Jul 17, 2015 at 08:19:16PM -0400, Programmingkid wrote:
>> @@ -2014,7 +2015,9 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex ma
>>         if ( bsdPathAsCFString ) {
>>             size_t devPathLength;
>>             strcpy( bsdPath, _PATH_DEV );
>> -            strcat( bsdPath, "r" );
>> +            if (flags & BDRV_O_NOCACHE) {
>> +                strcat(bsdPath, "r");
>> +            }
>>             devPathLength = strlen( bsdPath );
>>             if ( CFStringGetCString( bsdPathAsCFString, bsdPath + devPathLength, maxPathSize - devPathLength, kCFStringEncodingASCII ) ) {
>>                 kernResult = KERN_SUCCESS;
> 
> This hunk should be a separate patch.  

If I made the changes to the GetBSDPath() function its own patch, QEMU would no longer compile. The addition of a flags argument would cause this issue.
Stefan Hajnoczi July 27, 2015, 10:27 a.m. UTC | #3 
On Fri, Jul 24, 2015 at 11:37:50AM -0400, Programmingkid wrote:
> 
> On Jul 24, 2015, at 11:00 AM, Stefan Hajnoczi wrote:
> 
> > On Fri, Jul 17, 2015 at 08:19:16PM -0400, Programmingkid wrote:
> >> @@ -2014,7 +2015,9 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex ma
> >>         if ( bsdPathAsCFString ) {
> >>             size_t devPathLength;
> >>             strcpy( bsdPath, _PATH_DEV );
> >> -            strcat( bsdPath, "r" );
> >> +            if (flags & BDRV_O_NOCACHE) {
> >> +                strcat(bsdPath, "r");
> >> +            }
> >>             devPathLength = strlen( bsdPath );
> >>             if ( CFStringGetCString( bsdPathAsCFString, bsdPath + devPathLength, maxPathSize - devPathLength, kCFStringEncodingASCII ) ) {
> >>                 kernResult = KERN_SUCCESS;
> > 
> > This hunk should be a separate patch.  
> 
> If I made the changes to the GetBSDPath() function its own patch, QEMU would no longer compile. The addition of a flags argument would cause this issue.

Please include the addition of the flags argument and the change to the
call site in the separate patch.

This is a single logical change which deserves its own commit.  That way
you can explain that /dev/cdrom was opening the raw char device but not
probing alignment, causing bdrv_read() to fail.
diff mbox

Patch

diff --git a/block/raw-posix.c b/block/raw-posix.c
index cbe6574..5e4ddda 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -42,9 +42,8 @@ 
 #include <IOKit/storage/IOMediaBSDClient.h>
 #include <IOKit/storage/IOMedia.h>
 #include <IOKit/storage/IOCDMedia.h>
-//#include <IOKit/storage/IOCDTypes.h>
 #include <CoreFoundation/CoreFoundation.h>
-#endif
+#endif /* (__APPLE__) && (__MACH__) */
 
 #ifdef __sun__
 #define _POSIX_PTHREAD_SEMANTICS 1
@@ -1972,8 +1971,9 @@  BlockDriver bdrv_file = {
 /* host device */
 
 #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 );
+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 )
 {
@@ -2001,7 +2001,8 @@  kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
     return kernResult;
 }
 
-kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex maxPathSize )
+kern_return_t GetBSDPath(io_iterator_t mediaIterator, char *bsdPath,
+                         CFIndex maxPathSize, int flags)
 {
     io_object_t     nextMedia;
     kern_return_t   kernResult = KERN_FAILURE;
@@ -2014,7 +2015,9 @@  kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex ma
         if ( bsdPathAsCFString ) {
             size_t devPathLength;
             strcpy( bsdPath, _PATH_DEV );
-            strcat( bsdPath, "r" );
+            if (flags & BDRV_O_NOCACHE) {
+                strcat(bsdPath, "r");
+            }
             devPathLength = strlen( bsdPath );
             if ( CFStringGetCString( bsdPathAsCFString, bsdPath + devPathLength, maxPathSize - devPathLength, kCFStringEncodingASCII ) ) {
                 kernResult = KERN_SUCCESS;
@@ -2027,7 +2030,35 @@  kern_return_t GetBSDPath( io_iterator_t mediaIterator, char *bsdPath, CFIndex ma
     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++) {
+        pstrcpy(testPartition, strlen(bsdPath)+1, bsdPath);
+        snprintf(testPartition, MAXPATHLEN, "%ss%d", testPartition, 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, strlen(testPartition)+1, testPartition);
+    }
+    return partitionFound;
+}
+#endif /* defined(__APPLE__) && defined(__MACH__) */
 
 static int hdev_probe_device(const char *filename)
 {
@@ -2115,34 +2146,33 @@  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( &mediaIterator );
-        kernResult = GetBSDPath( mediaIterator, bsdPath, sizeof( bsdPath ) );
-
-        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(&mediaIterator);
+            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 );
+        qdict_put(options, "filename", qstring_from_str(filename));
     }
 #endif
 
@@ -2153,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("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",
+                                                                    filename);
+        printf("Command to mount device: diskutil mountDisk %s\n", filename);
+    }
+#endif /* defined(__APPLE__) && defined(__MACH__) */
+
+    if (ret < 0) {
+       return ret;
     }
 
     /* Since this does ioctl the device must be already opened */