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

[Eric Blake]

On 12/23/2015 04:24 AM, Daniel P. Berrange wrote:
> On Tue, Dec 22, 2015 at 11:45:45AM -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.
>>
>> [meta-review of JUST qapi decisions; code review in a separate message]
>>

>>
>> With your addition, ChardevFile now inherits from ChardevCommon, so we gain:
>>
>> { "execute": "chardev-add", "arguments": {
>>     "id": "foo", "backend": { "type": "file",
>>       "data": { "logfile": "logname", "out": "filename" } } } }
> 
> Ok good that matches what I intended - namely that 'logfile'
> should appear at the same dict as the rest of the backend
> fields, to mirror the the fact that the C struct had all
> the common fields in the same struct too.

Or in C terms, your proposal is:

struct ChardevCommon {
    char *logname; ...
};
struct ChardevFile {
    /* inherited fields from ChardevCommon */
    char *logname; ...
    /* own fields */
    char *out; ...
};
struct ChardevBackend {
    ChardevBackendKind type;
    union {
        ChardevFile *file; ...
    } u;
};

Each branch of ChardevBackend.u then has an upcast function
qapi_ChardevFile_base() that gets you to a ChardevCommon pointer.

> 
>> Re-writing the existing ChardevBackend to a semantically-identical flat
>> union would be (using my shorthand syntax for anonymous base [1] and
>> anonymous branch wrappers [2]):
>>

>>
>> Your proposal, then, is that the new 'logging' fields in your
>> ChardevCommon should be made part of the base type of the
>> 'ChardevBackend' union; which would be spelled as:
>>
>> { 'enum': 'ChardevType', 'data': [ 'file', 'serial' ] }
>> { 'struct': 'ChardevCommon', 'data': {
>>   'type': 'ChardevType', '*logfile': 'str', '*logappend': bool } }
>> { 'union': 'ChardevBackend',
>>   'base': 'ChardevCommon', 'discriminator': 'type',
>>   'data': { 'file': { 'data': 'ChardevFile' },
>>             'serial': { 'data': 'ChardevHostdev' } } }

In C terms, this one would be:

struct ChardevCommon {
    char *logname; ...
};
struct ChardevFile {
    char *out; ...
};
struct ChardevBackend {
    /* inherited fields from ChardevCommon */
    char *logname; ...
    /* own fields */
    ChardevBackendKind type;
    union {
        ChardevFile *file; ...
    } u;
};

Here, you can pass ChardevBackend* directly (and access
backend->logname, regardless of which union branch is in use).

>> So, depending on which of those three places we want to stick the new
>> parameters determines which approach we should use for exposing them in
>> qapi.
> 
>>From the QMP representation POV, my preference is for the current
> design since I think the 'logfile' attribute is really just another
> one of the backend config attributes. The choice to have a ChardevCommon
> struct was just a mechanism to avoid having to repeat the 'logfile'
> parameter in every single Chardev* backend type. This naturally
> matches the C-struct, where the ChardevCommon fields get directly
> added to the ChardevFile, ChardevSocket, etc structs.

Yep - the decision is up to you whether to add it to each struct used as
a branch of ChardevBackend (and each caller then upcasts and passes a
ChardevCommon* to the common code), or whether to add it directly to
ChardevBackend (and each caller merely passes a ChardevBackend*).

> 
> So from that POV, I'd be against, pushing the 'logfile' field up
> either 1 or 2 levels.
> 
> Regards,
> Daniel
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature