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
