Re: [Qemu-devel] [PATCH v2] qemu-char: add logfile facility to all chardev backends

[Eric Blake]

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