On 11/25/2015 05:24 PM, Programmingkid wrote: > 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> > > ---
Right here (between the --- and diffstat) it's nice to post a changelog
of how v8 differs from v7, to help earlier reviewers focus on the
improvements.
> block/raw-posix.c | 98
> +++++++++++++++++++++++++++++++++++++++--------------
> 1 files changed, 72 insertions(+), 26 deletions(-)
> +++ 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__) */
>
This hunk looks to be an unrelated cleanup; you might want to just
propose it separately through the qemu-trivial queue (but don't forget
that even trivial patches must cc qemu-devel).
> +
> + /* look for a working partition */
> + for (index = 0; index < num_of_test_partitions; index++) {
> + snprintf(test_partition, sizeof(test_partition), "%ss%d", bsd_path,
> +
> index);
Unusual indentation. More typical would be:
snprintf(test_partition, sizeof(test_partition), "%ss%d",
bsd_path, index);
with the second line flush to the character after the ( of the first line.
> + fd = qemu_open(test_partition, O_RDONLY | O_BINARY | O_LARGEFILE);
Isn't qemu_open() supposed to provide O_LARGEFILE for ALL users
automatically? (That is, why would we ever _want_ to handle a file
using only 32-bit off_t?) But that's a separate issue; it looks like
you are copy-and-pasting from existing use of this idiom already in
raw-posix.c.
> + if (fd >= 0) {
> + partition_found = true;
> + qemu_close(fd);
> + break;
> + }
> + }
> +
> + /* if a working partition on the device was not found */
> + if (partition_found == false) {
> + error_setg(errp, "Error: Failed to find a working partition on "
> +
> "disc!\n");
Several violations of convention. error_setg() does not need a
redundant "Error: " prefix, should not end in '!' (we aren't shouting),
and should not end in newline. And with those fixes, you won't even
need the weird indentation.
error_setg(errp, "failed to find a working partition on disk");
>
> +/* Prints directions on mounting and unmounting a device */
> +static void printUnmountingDirections(const char *file_name)
Elsewhere, we use 'function_name', not 'functionName'.
> +{
> + error_report("Error: If device %s is mounted on the desktop, unmount"
> + " it first before using it in QEMU.\n",
> file_name);
> + error_report("Command to unmount device: diskutil unmountDisk %s\n",
> +
> file_name);
> + error_report("Command to mount device: diskutil mountDisk %s\n",
> +
> file_name);
Again, weird indentation. And don't use \n at the end of error_report().
> @@ -2123,32 +2162,32 @@ static int hdev_open(BlockDriverState *bs, QDict
> *options, int flags,
> int ret;
>
> #if defined(__APPLE__) && defined(__MACH__)
> - const char *filename = qdict_get_str(options, "filename");
> + const char *file_name = qdict_get_str(options, "filename");
No need to rename this variable.
> +
> + /* If a real optical drive was not found */
> + if (bsd_path[0] == '\0') {
> + error_setg(errp, "Error: failed to obtain bsd path for optical"
> + "
> drive!\n");
Again, weird indentation, redundant "Error: ", and no "!\n" at the end.
>
> +#if defined(__APPLE__) && defined(__MACH__)
> + /* if a physical device experienced an error while being opened */
> + if (strncmp(file_name, "/dev/", 5) == 0 && ret != 0) {
> + printUnmountingDirections(file_name);
Is this advice appropriate to ALL things under /dev/, or just cdroms?
> + return -1;
> + }
> +#endif /* defined(__APPLE__) && defined(__MACH__) */
> +
> /* Since this does ioctl the device must be already opened */
> bs->sg = hdev_is_sg(bs);
>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
