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

2015-07-27 Thread Stefan Hajnoczi 
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.


pgp0YfK0eZU0m.pgp
Description: PGP signature

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

2015-07-24 Thread Stefan Hajnoczi 
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.


pgpMGjp0z3tci.pgp
Description: PGP signature

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

2015-07-17 Thread 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 programmingk...@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(-)

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;
+