[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v9] block/raw-posix.c: Make physica
From: |
Programmingkid |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v9] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host |
Date: |
Fri, 27 Nov 2015 14:35:46 -0500 |
On Nov 25, 2015, at 11:23 PM, Eric Blake wrote:
> On 11/25/2015 09:10 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 <address@hidden>
>>
>> ---
>> Added DVD support - real DVD media can now be used in QEMU!
>> Changed name of FindEjectableCDMedia to FindEjectableOpticalMedia.
>> Added mediaType parameter to FindEjectableOpticalMedia() for media
>> indentification.
>> Removed unneeded FindEjectableCDMedia() prototype.
>> FindEjectableOpticalMedia() now searches for both DVD's and CD's.
>>
>
>> +++ b/block/raw-posix.c
>> @@ -42,9 +42,9 @@
>> #include <IOKit/storage/IOMediaBSDClient.h>
>> #include <IOKit/storage/IOMedia.h>
>> #include <IOKit/storage/IOCDMedia.h>
>> -//#include <IOKit/storage/IOCDTypes.h>
>> +#include <IOKit/storage/IODVDMedia.h>
>> #include <CoreFoundation/CoreFoundation.h>
>> -#endif
>> +#endif /* (__APPLE__) && (__MACH__) */
>>
>
> I don't know if my review of v8 crossed your posting of this patch, but
> I suggested that this hunk belongs in its own patch.
It is now a related change that the code in the patch depends on.
>
>> #ifdef __sun__
>> #define _POSIX_PTHREAD_SEMANTICS 1
>> @@ -1975,32 +1975,44 @@ 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, int flags);
>> -kern_return_t FindEjectableCDMedia( io_iterator_t *mediaIterator )
>> +static kern_return_t FindEjectableOpticalMedia(io_iterator_t *mediaIterator,
>> + char
>> *mediaType)
>
> Unusual indentation; more typical is:
>
> | static kern_return_t FindEjectableOpticalMedia(io_iterator_t
> *mediaIterator,
> | char *mediatType)
I agree. I wanted the second long to be right justified with the 80 character
line count.
>
>
>> + int index;
>> + for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
>> + classesToMatch = IOServiceMatching(matching_array[index]);
>> + if (classesToMatch == NULL) {
>> + printf("IOServiceMatching returned a NULL dictionary.\n");
>
> Leftover debugging?
It is actually how the function was originally written. It probably needs to be
improved.
Should I replace printf() with error_report()?
>
>> + } else {
>> + CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
>
> Missing indentation.
Fixed in the next patch.
>
>> +
>> kCFBooleanTrue);
>> + }
>> + kernResult = IOServiceGetMatchingServices(masterPort,
>> classesToMatch,
>> +
>> mediaIterator);
>
> More unusual indentation.
Fixed in the next patch.
>
>> + if (KERN_SUCCESS != kernResult) {
>> + printf("IOServiceGetMatchingServices returned %d\n",
>> kernResult);
>> + }
>>
>> + /* If you found a match, leave the loop */
>> + if (*mediaIterator != 0) {
>> + DPRINTF("Matching using %s\n", matching_array[index]);
>> + snprintf(mediaType, strlen(matching_array[index])+1, "%s",
>
> Spaces around binary '+'.
What's wrong with no spaces around the plus sign?
>
>> + /* 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");
>
> and I already pointed out on v8 that this is not the correct usage of
> error_setg(). So, here's hoping v10 addresses the comments here and
> elsewhere.
Kevin Wolf wanted it this way. What would you do instead?
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
Thank you very much for reviewing my patches.