On 12/23/2015 04:32 AM, Daniel P. Berrange wrote: > On Tue, Dec 22, 2015 at 12:07:03PM -0700, Eric Blake wrote: >> On 12/22/2015 11:17 AM, Daniel P. Berrange wrote: >>> Typically a UNIX guest OS will log boot messages to a serial >>> port in addition to any graphical console. An admin user >>> may also wish to use the serial port for an interactive >>> console. A virtualization management system may wish to >>> collect system boot messages by logging the serial port, >>> but also wish to allow admins interactive access. >>> >> >> [code review, if we go with this design; see my other message for 2 >> possible alternative qapi designs that may supersede this code review] >>
>>> ##
>>> -{ 'struct': 'ChardevDummy', 'data': { } }
>>> +{ 'struct': 'ChardevDummy', 'data': { },
>>> + 'base': 'ChardevCommon' }
>>
>> Instead of changing ChardevDummy, you could have deleted this type and done:
>>
>>>
>>> { 'union': 'ChardevBackend', 'data': { 'file' : 'ChardevFile',
>>> 'serial' : 'ChardevHostdev',
>> ...
>> 'pty': 'ChardevCommon',
>> 'null': 'ChardevCommon',
>>
>> and so on. I don't know which is better.
>
> Yep, me neither. Given the name it felt like some kind of placeholder
> for future work, so I left it alone.
ChardevDummy exists because we have no way (yet) to represent an empty
type as a union branch. That is, since you can't do:
'pty': {},
we had to instead create a named empty type. But now that we have a
non-empty type, I see no real reason to keep the name, and no reason to
have a subclass that adds no additional fields; so dropping ChardevDummy
is my recommendation.
>>
>> The other thing we could do here is have qemu_chr_open_common() take a
>> ChardevCommon* instead of a ChardevBackend*. Then each caller would do
>> an appropriate upcast before calling the common code:
>>
>> ChardevCommon *common = qapi_ChardevDummy_base(backend->u.null);
>> if (qemu_chr_open_common(chr, common, errp) {
>
> Yep, I think this is the easiest thing todo, to avoid use of
> backend->u.data.
>
>> and so forth. That feels a bit more type-safe (and less reliant on qapi
>> layout guarantees) than trying to rely on the backend->u.data that I'm
>> trying to get rid of.
>
> Agreed
>
Okay, I think having each branch be a subclass of ChardevCommon (and not
ChardevBackend using ChardevCommon as a base) sounds like the way to go,
and now it's up to v3 to rework the clients to be a bit more typesafe.
>>> +++ b/qemu-options.hx
>>> @@ -2034,40 +2034,43 @@ The general form of a character device option is:
>>> ETEXI
>>>
>>> DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
>>> - "-chardev null,id=id[,mux=on|off]\n"
>>> + "-chardev null,id=id[,mux=on|off][,logfile=PATH][,logappend=on|off]\n"
>>
>> Do we want to allow logappend=on even when logfile is unspecified, or
>> should we make that an error?
>
> I figured it was harmless to just ignore it.
Works for me.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
