From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45690) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1RXl-0003Rz-3Z for qemu-devel@nongnu.org; Tue, 24 Nov 2015 23:18:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1RXh-0003Ox-VX for qemu-devel@nongnu.org; Tue, 24 Nov 2015 23:18:57 -0500 Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: text/plain; charset=us-ascii From: Programmingkid In-Reply-To: <20151124143819.GE4296@noname.str.redhat.com> Date: Tue, 24 Nov 2015 23:18:50 -0500 Content-Transfer-Encoding: quoted-printable Message-Id: <893DD1CA-B5EE-41C7-AFDA-32EAC1C71D69@gmail.com> References: <28524702-D15C-4EC2-888B-74140E572988@gmail.com> <20151124143819.GE4296@noname.str.redhat.com> Subject: Re: [Qemu-devel] [PATCH v6] 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: Kevin Wolf Cc: qemu-devel qemu-devel , Qemu-block On Nov 24, 2015, at 9:38 AM, Kevin Wolf wrote: >=20 >=20 >> + /* 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") =3D=3D 0) { >=20 > 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.=20 >=20 > 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. >=20 >> + io_iterator_t mediaIterator; >> + FindEjectableCDMedia(&mediaIterator); >> + GetBSDPath(mediaIterator, bsdPath, sizeof(bsdPath), = flags); >> + if (bsdPath[0] =3D=3D '\0') { >> + printf("Error: failed to obtain bsd path for optical = drive!\n"); >=20 > 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.=20 >>=20 >> +#if defined(__APPLE__) && defined(__MACH__) >> + /* if a physical device experienced an error while being opened = */ >> + if (strncmp(filename, "/dev/", 5) =3D=3D 0 && (cdromOK =3D=3D = false || ret !=3D 0)) { >=20 > 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.=20 >=20 > 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.=