Re: [Qemu-devel] [PATCH v3 47/50] paaudio: channel-map option

From: Eric Blake <eblake@redhat.com>
To: "Zoltán Kővágó" <dirty.ice.hu@gmail.com>,
	"Gerd Hoffmann" <kraxel@redhat.com>
Cc: qemu-devel@nongnu.org, Markus Armbruster <armbru@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v3 47/50] paaudio: channel-map option
Date: Wed, 23 Jan 2019 14:33:48 -0600	[thread overview]
Message-ID: <620cb622-56a6-5020-f152-1291840c3ca7@redhat.com> (raw)
In-Reply-To: <23779539-c590-3910-7cf3-8e2de29ab9b9@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]

On 1/23/19 2:13 PM, Zoltán Kővágó wrote:

>>>  { 'struct': 'AudiodevPaPerDirectionOptions',
>>>    'data': {
>>> -    '*name': 'str' } }
>>> +    '*name':        'str',
>>> +    '*channel-map': 'str' } }
>>
>> Ah, I see.  Thats why patch #1 creates a AudiodevPaPerDirectionOptions
>> struct with just one field ...
> 
> Yes, I was bitten by it too during a refactor, probably I should write a
> few words about it in the commit message.

Yes, a good commit message tries to anticipate the questions the
reviewer will ask. The "what" is easy to see (read the patch), but the
"why" is the one that determines whether the patch is worth including.

> 
> However, this is the exact reason I'd recommend nested structs instead
> of randomly flattening it when we can.  This way, if we later have to
> add an extra option, we don't end up in a problematic situation, since
> we can't easily change things like 'dev-in' to a structure without
> breaking backward compatibility.

We can support QAPI Alternates, accepting either a string or a struct
for a given member name like 'dev-in'. Admittedly, it's a bit harder to
write C code interfacing with a QAPI alternate type, but at least it is
possible and allows for back-compatibility.  Then again, getting the
struct right in the first place is even nicer than having to make
back-compat tweaks.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]