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

[Daniel P. Berrange]

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]
> 
> > A simpler approach that is satisfactory for many use
> > cases is to allow the QEMU chardev backends to have a
> > "logfile" property associated with them.
> > 
> >  $QEMU -chardev socket,host=localhost,port=9000,\
> >                 server=on,nowait,id-charserial0,\
> >             logfile=/var/log/libvirt/qemu/test-serial0.log
> >        -device isa-serial,chardev=charserial0,id=serial0
> > 
> > This patch introduces a 'ChardevCommon' struct which
> > is setup as a base for all the ChardevBackend types.
> > Ideally this would be registered directly as a base
> > against ChardevBackend, rather than each type, but
> > the QAPI generator doesn't allow that since the
> > ChardevBackend is a non-discriminated union. The
> > ChardevCommon struct provides the optional 'logfile'
> > parameter, as well as 'logappend' which controls
> > whether QEMU truncates or appends (default truncate).
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> > 
> 
> > +++ b/qapi-schema.json
> > @@ -3093,6 +3093,21 @@
> >  ##
> >  { 'command': 'screendump', 'data': {'filename': 'str'} }
> >  
> > +
> > +##
> > +# @ChardevCommon:
> > +#
> > +# Configuration shared across all chardev backends
> > +#
> > +# @logfile: #optional The name of a logfile to save output
> > +# @logappend: #optional true to append instead of truncate
> > +#             (default to false to truncate)
> 
> Could shorten to:
> 
> # @logappend: #optional true to append to @logfile (default false to
> truncate)
> 
> >  ##
> >  # @ChardevBackend:
> > @@ -3243,7 +3269,8 @@
> >  #
> >  # Since: 1.4 (testdev since 2.2)
> >  ##
> > -{ '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.


> > +/* Not reporting errors from writing to logfile, as logs are
> > + * defined to be "best effort" only */
> > +static void qemu_chr_fe_write_log(CharDriverState *s,
> > +                                  const uint8_t *buf, size_t len)
> > +{
> > +    size_t done = 0;
> > +    ssize_t ret;
> > +
> > +    if (s->logfd < 0) {
> > +        return;
> > +    }
> > +
> > +    while (done < len) {
> > +        do {
> > +            ret = write(s->logfd, buf + done, len - done);
> > +            if (ret == -1 && errno == EAGAIN) {
> > +                g_usleep(100);
> > +            }
> 
> Do we really want to be sleeping here?

If logfile points to a plain file, you'll never get EAGAIN.
For that matter even if it doesn't point to a plain file
I realize that we've not called qemu_nonblock() on logfd,
so it'll never get EAGAIN for that reason either.

The qemu_chr_fe_write_all() currently has the same usleep
which is what I followed. The qemu_chr_fe_write() just
returns on EAGAIN.  In the write_log() method we want to
try to write all the data the qemu_chr_fe_write/write_all
just sent. If we ignore EGAIN, we'll potentially loose
data from the log, but if we honour it, we have to sleep
a little or not set O_NONBLOCK.

> 
> > +        } while (ret == -1 && errno == EAGAIN);
> > +
> > +        if (ret <= 0) {
> 
> Why '<='?  Shouldn't this be '<'?

Well there's no difference really since write() will never
return 0.

> 
> > +            return;
> > +        }
> > +        done += ret;
> > +    }
> > +}
> > +
> 
> >  
> > +
> > +static int qemu_chr_open_common(CharDriverState *chr,
> > +                                ChardevBackend *backend,
> > +                                Error **errp)
> > +{
> > +    ChardevCommon *common = backend->u.data;
> 
> Phooey.  This conflicts with my pending patches to get rid of 'data':
> 
> http://repo.or.cz/qemu/ericb.git/commitdiff/84aaab99
> 
> I mentioned above that you could do things like 'null':'ChardevCommon'
> instead of 'null':'ChardevDummy'; and we also know that qapi guarantees
> a layout where all base types occur at the front of the rest of the
> type.  So you could write this as:
> 
> ChardevCommon *common = backend->u.null;
> 
> and things will work even when 'data' is gone.  But maybe that argues
> more strongly that we should hoist the common members into the base
> fields of ChardevBackend struct, or even as separate parameters to the
> 'chardev-add' command (the two alternate qapi representations I
> described in my other message).
> 
> > +
> > +    if (common->has_logfile) {
> > +        int flags = O_WRONLY | O_CREAT;
> > +        if (!common->has_logappend ||
> > +            !common->logappend) {
> > +            flags |= O_TRUNC;
> > +        }
> 
> Umm, don't you need to set O_APPEND when common->logappend is true?
> 
> > +        chr->logfd = qemu_open(common->logfile, flags, 0666);
> ...
> > @@ -367,9 +432,16 @@ static CharDriverState *qemu_chr_open_null(const char 
> > *id,
> >      CharDriverState *chr;
> >  
> >      chr = qemu_chr_alloc();
> > +    if (qemu_chr_open_common(chr, backend, errp) < 0) {
> > +        goto error;
> > +    }
> 
> 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

> > +++ 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.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|