Re: [Qemu-devel] [Qemu-block] ping [PATCH v11] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
On 12/11/2015 04:40 PM, Programmingkid wrote:
> I guess the commit message is a little out of date. How about this:
>
> 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. Also new to this patch is the ability to use DVD
> disc.
>
The use of "Also" in a commit message is often a sign of trying to do
too much in one patch. If you are doing two distinct things (adding a
new message, vs. adding the ability to use a DVD disk), then two patches
is probably the best way to approach it.
>> Also, this patch seems garbled. When saved via Mutt, or when I view
>> it "raw", there are '=20" and '=3D' all around, a sure sign that it is
>> (or once was) not plaintext.
>>
>> I recommend using git format-patch [1] and git send-email [1] to
>> create and send patches - it'll do the Right Thing.
>
> You really see that? I don't know why. The link I provide to the patch in
> patchworks shows no problems. The patch itself was sent as a plain text
> file.
Your mail was sent with:
> Content-Type: text/plain; charset=us-ascii
> Content-Transfer-Encoding: quoted-printable
> Mime-Version: 1.0 (Apple Message framework v1084)
and the body indeed contains quoted-printable text, such as:
> @@ -1975,32 +1976,46 @@ BlockDriver bdrv_file =3D {
> /* host device */
> =20
Many mailers are good enough to re-render things on the fly
(Thunderbird, for example, does not have those annoying =3D and =20
insertions in the displayed text) - but what gets rendered in the mail
client is different than what the mail client saves to file.
Mails sent by 'git send-email', on the other hand, use:
> MIME-Version: 1.0
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
Apparently, mutt doesn't quite defang a quoted-printable message
properly, and/or 'git am' doesn't take too well to a quoted-printable
message when trying to apply a saved file as a patch. Which is all the
more reason why using the same tool as everyone else makes sure that
your patches will interoperate with the receivers' tools the same as the
receiver is already used to handling other patches.
>> Just use g_strdup().
>
> I like snprintf() because it very well documented.
So is g_strdup(). But more important than being well-documented,
g_strdup() also has the benefit of being correctly usable with less
boilerplate - you can accomplish the same task in fewer lines of code,
and focus the reviewer on your task rather than on the boilerplate.
That, and I've seen a number of programs that incorrectly use snprintf()
(such as incorrectly sizing a buffer, or failing to check for sizing
errors after the fact). An interface that is hard to use correctly
should be avoided in favor of one that is less mental effort.
>
> Is this what you had in mind:
>
> mediaType = g_strdup(matching_array[index]);
Pretty much, coupled with g_free(mediaType) later on.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [Qemu-block] ping [PATCH v11] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
On Dec 11, 2015, at 5:00 PM, Jeff Cody wrote:
> On Thu, Dec 10, 2015 at 09:39:51AM -0500, Programmingkid wrote:
>> https://patchwork.ozlabs.org/patch/550295/
>>
>> 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.
>>
>
> This commit message doesn't seem to really match the patch; it is more
> than just a message displayed to the user. For instance, before it
> looked for just kIOCDMediaClass - now, it searches for kIOCDMediaClass
> and kIODVDMediaClass.
I guess the commit message is a little out of date. How about this:
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. Also new to this patch is the ability to use DVD
disc.
>
>> Signed-off-by: John Arbuckle
>>
>> ---
>> error_report()'s had their \n, '.', and "Error:" removed.
>> Indentations are now at the left parenthesis rather than
>> at the 80 character mark.
>> Added spaces between the + sign.
>>
>
> Also, this patch seems garbled. When saved via Mutt, or when I view
> it "raw", there are '=20" and '=3D' all around, a sure sign that it is
> (or once was) not plaintext.
>
> I recommend using git format-patch [1] and git send-email [1] to
> create and send patches - it'll do the Right Thing.
You really see that? I don't know why. The link I provide to the patch in
patchworks shows no problems. The patch itself was sent as a plain text
file.
>
>> block/raw-posix.c | 135
>> +++--
>> 1 files changed, 99 insertions(+), 36 deletions(-)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index ccfec1c..39e523b 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -43,6 +43,7 @@
>> #include
>> #include
>> //#include
>> +#include
>> #include
>> #endif
>>
>> @@ -1975,32 +1976,46 @@ 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)
>> {
>> kern_return_t kernResult;
>> mach_port_t masterPort;
>> CFMutableDictionaryRef classesToMatch;
>> +const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
>>
>> kernResult = IOMasterPort( MACH_PORT_NULL, &masterPort );
>> if ( KERN_SUCCESS != kernResult ) {
>> printf( "IOMasterPort returned %d\n", kernResult );
>> }
>>
>> -classesToMatch = IOServiceMatching( kIOCDMediaClass );
>> -if ( classesToMatch == NULL ) {
>> -printf( "IOServiceMatching returned a NULL dictionary.\n" );
>> -} else {
>> -CFDictionarySetValue( classesToMatch, CFSTR( kIOMediaEjectableKey ),
>> kCFBooleanTrue );
>> -}
>> -kernResult = IOServiceGetMatchingServices( masterPort, classesToMatch,
>> mediaIterator );
>> -if ( KERN_SUCCESS != kernResult )
>> -{
>> -printf( "IOServiceGetMatchingServices returned %d\n", kernResult );
>> -}
>> +int index;
>> +for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
>> +classesToMatch = IOServiceMatching(matching_array[index]);
>> +if (classesToMatch == NULL) {
>> +error_report("IOServiceMatching returned NULL for %s",
>> + matching_array[index]);
>> +continue;
>> +}
>> +CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
>> + kCFBooleanTrue);
>> +kernResult = IOServiceGetMatchingServices(masterPort,
>> classesToMatch,
>> + mediaIterator);
>> +if (kernResult != KERN_SUCCESS) {
>> +error_report("Note: IOServiceGetMatchingServices returned %d",
>> + kernResult);
>> +}
>>
>> +/* If a match was found, leave the loop */
>> +if (*mediaIterator != 0) {
>
> Since mediaIterator was not ever initialized in hdev_open, we can't
> assume the value of mediaIterator as being 0 after kernResult !=
> KERN_SUCCESS, can we? A quick google search [3] shows it ambiguous, so
> best initialize mediaIterator down below...
Sounds like a good suggestion.
>
>
>> +DPRINTF("Matching using %s\n", matching_array[index]);
>> +snprintf(mediaType, strlen(matching_array[index]) + 1, "%s",
>> +
Re: [Qemu-devel] [Qemu-block] ping [PATCH v11] block/raw-posix.c: Make physical devices usable in QEMU under Mac OS X host
On Thu, Dec 10, 2015 at 09:39:51AM -0500, Programmingkid wrote:
> https://patchwork.ozlabs.org/patch/550295/
>
> 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.
>
This commit message doesn't seem to really match the patch; it is more
than just a message displayed to the user. For instance, before it
looked for just kIOCDMediaClass - now, it searches for kIOCDMediaClass
and kIODVDMediaClass.
> Signed-off-by: John Arbuckle
>
> ---
> error_report()'s had their \n, '.', and "Error:" removed.
> Indentations are now at the left parenthesis rather than
> at the 80 character mark.
> Added spaces between the + sign.
>
Also, this patch seems garbled. When saved via Mutt, or when I view
it "raw", there are '=20" and '=3D' all around, a sure sign that it is
(or once was) not plaintext.
I recommend using git format-patch [1] and git send-email [1] to
create and send patches - it'll do the Right Thing.
> block/raw-posix.c | 135
> +++--
> 1 files changed, 99 insertions(+), 36 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index ccfec1c..39e523b 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -43,6 +43,7 @@
> #include
> #include
> //#include
> +#include
> #include
> #endif
>
> @@ -1975,32 +1976,46 @@ 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)
> {
> kern_return_t kernResult;
> mach_port_t masterPort;
> CFMutableDictionaryRef classesToMatch;
> +const char *matching_array[] = {kIODVDMediaClass, kIOCDMediaClass};
>
> kernResult = IOMasterPort( MACH_PORT_NULL, &masterPort );
> if ( KERN_SUCCESS != kernResult ) {
> printf( "IOMasterPort returned %d\n", kernResult );
> }
>
> -classesToMatch = IOServiceMatching( kIOCDMediaClass );
> -if ( classesToMatch == NULL ) {
> -printf( "IOServiceMatching returned a NULL dictionary.\n" );
> -} else {
> -CFDictionarySetValue( classesToMatch, CFSTR( kIOMediaEjectableKey ),
> kCFBooleanTrue );
> -}
> -kernResult = IOServiceGetMatchingServices( masterPort, classesToMatch,
> mediaIterator );
> -if ( KERN_SUCCESS != kernResult )
> -{
> -printf( "IOServiceGetMatchingServices returned %d\n", kernResult );
> -}
> +int index;
> +for (index = 0; index < ARRAY_SIZE(matching_array); index++) {
> +classesToMatch = IOServiceMatching(matching_array[index]);
> +if (classesToMatch == NULL) {
> +error_report("IOServiceMatching returned NULL for %s",
> + matching_array[index]);
> +continue;
> +}
> +CFDictionarySetValue(classesToMatch, CFSTR(kIOMediaEjectableKey),
> + kCFBooleanTrue);
> +kernResult = IOServiceGetMatchingServices(masterPort, classesToMatch,
> + mediaIterator);
> +if (kernResult != KERN_SUCCESS) {
> +error_report("Note: IOServiceGetMatchingServices returned %d",
> + kernResult);
> +}
>
> +/* If a match was found, leave the loop */
> +if (*mediaIterator != 0) {
Since mediaIterator was not ever initialized in hdev_open, we can't
assume the value of mediaIterator as being 0 after kernResult !=
KERN_SUCCESS, can we? A quick google search [3] shows it ambiguous, so
best initialize mediaIterator down below...
> +DPRINTF("Matching using %s\n", matching_array[index]);
> +snprintf(mediaType, strlen(matching_array[index]) + 1, "%s",
> + matching_array[index]);
Just use g_strdup().
We use snprintf here, with a size limit of the string referenced in
the array, which references a string defined in an OS X system header...
> +break;
> +}
> +}
> return kernResult;
You may return garbage here, because kernResult is never initialized,
and your for() loop short circuits on a NULL return from
IOServiceMatching(). Does this compile with our flags? I don't have
OS X so I can't try it, but I thought at least with gcc and -werror, a
possible uninitialized usage would be flagged.
> }
>
> @@ -2033,7 +2048,35 @@ kern_return_t GetBSDPath(io_iterator_t mediaIterator,
> char *bsdPath,
> return k
