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 <berra...@redhat.com>
> > ---
> >
>
> > +++ 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 :|
