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