From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42164) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZG5sz-0000nw-G7 for qemu-devel@nongnu.org; Fri, 17 Jul 2015 09:41:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZG5sw-0001P6-8d for qemu-devel@nongnu.org; Fri, 17 Jul 2015 09:41:09 -0400 Date: Fri, 17 Jul 2015 14:41:01 +0100 From: Stefan Hajnoczi Message-ID: <20150717134101.GE6572@stefanha-thinkpad.redhat.com> References: <99ECAD58-C9CC-4780-B63C-AF975AECCED6@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="gMR3gsNFwZpnI/Ts" Content-Disposition: inline In-Reply-To: <99ECAD58-C9CC-4780-B63C-AF975AECCED6@gmail.com> Subject: Re: [Qemu-devel] [PATCH v2] 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 Cc: Laurent Vivier , Peter Maydell , qemu-devel qemu-devel , Qemu-block --gMR3gsNFwZpnI/Ts Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jul 16, 2015 at 04:46:07PM -0400, Programmingkid wrote: > @@ -2014,7 +2015,9 @@ kern_return_t GetBSDPath( io_iterator_t mediaIterat= or, 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 =3D strlen( bsdPath ); > if ( CFStringGetCString( bsdPathAsCFString, bsdPath + devPat= hLength, maxPathSize - devPathLength, kCFStringEncodingASCII ) ) { > kernResult =3D KERN_SUCCESS; Is this the fix that makes CD-ROM passthrough work for you? Does the guest boot successfully when you do: -drive if=3Dide,media=3Dcdrom,cache=3Dnone,file=3D/dev/cdrom ? > @@ -2027,7 +2030,67 @@ kern_return_t GetBSDPath( io_iterator_t mediaItera= tor, char *bsdPath, CFIndex ma > return kernResult; > } > =20 > -#endif > +/* Sets up a physical device for use in QEMU */ > +static void setupDevice(const char *bsdPath) > +{ > + /* > + * Mac OS X does not like allowing QEMU to use physical devices that = are > + * mounted. Attempts to do so result in 'Resource busy' errors. > + */ > + > + int fd; > + fd =3D qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); > + > + /* if the device fails to open */ > + if (fd < 0) { > + printf("Error: failed to open %s\n", bsdPath); > + printf("If device %s is mounted on the desktop, unmount it" > + " first before using it in QEMU.\n", bsdPath); > + printf("\nCommand to unmount device: diskutil unmountDisk %s", b= sdPath); > + printf("\nCommand to mount device: diskutil mountDisk %s\n\n", b= sdPath); > + } This error message is printed regardless of the errno value. What is the specific errno value when open(2) fails because the device is mounted? Also, can you move this after the raw_open_common() call to avoid the setupCDROM()/setupDevice() changes made in this patch and duplicating the error message. It doesn't seem necessary to open the files ahead of raw_open_common() since the code continues in the error case anyway: ret =3D raw_open_common(bs, options, flags, 0, &local_err); if (ret < 0) { if (local_err) { error_propagate(errp, local_err); } #if defined(__APPLE__) && defined(__MACH__) if (strstart(filename, "/dev/") =3D=3D 0 && ret =3D=3D -EBUSY) { /*= or whatever */ error_report("If device %s is mounted on the desktop, unmount it" " first before using it in QEMU.", bsdPath); error_report("Command to unmount device: diskutil unmountDisk %s", bsd= Path); error_report("Command to mount device: diskutil mountDisk %s", bsdPath= ); } #endif /* defined(__APPLE__) && defined(__MACH__) */ return ret; } That's a much smaller change. > + > + /* if the device opens */ > + else { > + qemu_close(fd); > + } > +} > + > +/* Sets up a real cdrom for use in QEMU */ > +static void setupCDROM(char *bsdPath) > +{ > + int index, numOfTestPartitions =3D 2, fd; > + char testPartition[MAXPATHLEN]; > + bool partitionFound =3D false; > + > + /* look for a working partition */ > + for (index =3D 0; index < numOfTestPartitions; index++) { > + strncpy(testPartition, bsdPath, MAXPATHLEN); The safe way to use strncpy() is: strncpy(testPartition, bsdPath, MAXPATHLEN - 1); testPartition[MAXPATHLEN - 1] =3D '\0'; but pstrcpy() is a easier to use correctly. Please use that instead. > + snprintf(testPartition, MAXPATHLEN, "%ss%d", testPartition, inde= x); > + fd =3D qemu_open(testPartition, O_RDONLY | O_BINARY | O_LARGEFIL= E); > + if (fd > 0) { Should be fd >=3D 0 since fd =3D 0 is valid too, although unlikely. > + partitionFound =3D true; > + qemu_close(fd); > + break; > + } > + } > + > + /* if a working partition on the device was not found */ > + if (partitionFound =3D=3D false) { > + printf("Error: Failed to find a working partition on disc!\n"); > + printf("If your disc is mounted on the desktop, trying unmountin= g it" > + " first before using it in QEMU.\n"); > + printf("\nCommand to unmount disc: " > + "diskutil unmountDisk %s\n", bsdPath); > + printf("Command to mount disc: " > + "diskutil mountDisk %s\n\n", bsdPath); > + } > + > + DPRINTF("Using %s as CDROM\n", testPartition); > + strncpy(bsdPath, testPartition, MAXPATHLEN); Please use pstrcpy(). > +} > + > +#endif /* defined(__APPLE__) && defined(__MACH__) */ > =20 > static int hdev_probe_device(const char *filename) > { > @@ -2119,30 +2182,28 @@ static int hdev_open(BlockDriverState *bs, QDict = *options, int flags, > #if defined(__APPLE__) && defined(__MACH__) > const char *filename =3D qdict_get_str(options, "filename"); > =20 > - if (strstart(filename, "/dev/cdrom", NULL)) { > - kern_return_t kernResult; > - io_iterator_t mediaIterator; > - char bsdPath[ MAXPATHLEN ]; > - int fd; > - > - kernResult =3D FindEjectableCDMedia( &mediaIterator ); > - kernResult =3D GetBSDPath( mediaIterator, bsdPath, sizeof( bsdPa= th ) ); > - > - if ( bsdPath[ 0 ] !=3D '\0' ) { > - strcat(bsdPath,"s0"); > - /* some CDs don't have a partition 0 */ > - fd =3D qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE); > - if (fd < 0) { > - bsdPath[strlen(bsdPath)-1] =3D '1'; > - } else { > - qemu_close(fd); > + /* If using a physical device */ > + if (strstart(filename, "/dev/", NULL)) { > + > + /* If the physical device is a cdrom */ > + if (strcmp(filename, "/dev/cdrom") =3D=3D 0) { > + char bsdPath[MAXPATHLEN]; > + io_iterator_t mediaIterator; > + FindEjectableCDMedia(&mediaIterator); > + GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath), flags); Why did you remove the if (bsdPath[0] !=3D [0]) check? Now the code ignored GetBSDPath() failures. > + if (mediaIterator) { > + IOObjectRelease(mediaIterator); > } > + setupCDROM(bsdPath); > filename =3D bsdPath; > - qdict_put(options, "filename", qstring_from_str(filename)); > } bsdPath is out of scope here so filename is a dangling pointer in the /dev/cdrom case. > =20 > - if ( mediaIterator ) > - IOObjectRelease( mediaIterator ); > + /* Setup any other physical device e.g. USB flash drive */ > + else { > + setupDevice(filename); > + } > + > + qdict_put(options, "filename", qstring_from_str(filename)); > } > #endif > =20 > --=20 > 1.7.5.4 >=20 --gMR3gsNFwZpnI/Ts Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVqQXtAAoJEJykq7OBq3PI3JkH+wUMJqCKKBFpc4FHhZ+qQh9b 9r8uCqtUl9Fxo0e24J0Wyar8ojgo+/0pR6TQxomiI+vqWTX7fyVxmzhR03AWx51x TsINbIFbG8NIrxh9nhacmBLbr2zzlsq7sqDcROzIeJnemhJkKbSciMLcusGrUtor HQcvj0Zhf1x8ka7p172ehxUj6z5/BCZXnKrUpTgcZSWh2inQJ38YukBhbpho+oda pKrsZ/nvCvZR/yaN98U1aPN1EApZjzASJQfnQCka06swNkSLPZ6fWTv+o4QlVFCx EUXuSS/B8SjfmKSQW5Poj6GC/RdKwtYZMevODaTw2S5HGkdyxVu5/j4C3a05mpQ= =eWCv -----END PGP SIGNATURE----- --gMR3gsNFwZpnI/Ts--