Re: [Qemu-devel] [PATCH v4 13/13] qga: start a man page

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Eric Blake (2015-08-28 16:44:49)
> On 08/26/2015 05:34 PM, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > Add a simple man page for the qemu agent.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > ---
> >  Makefile      |  14 +++++-
> >  qemu-doc.texi |   6 +++
> >  qemu-ga.texi  | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 154 insertions(+), 2 deletions(-)
> >  create mode 100644 qemu-ga.texi
> > 
> 
> > +++ b/qemu-ga.texi
> > @@ -0,0 +1,136 @@
> > +@example
> > +@c man begin SYNOPSIS
> > +usage: qemu-ga [-m <method> -p <path>] [OPTION]...
> 
> This implies that -m/-p are linked (either both or neither must be
> specified); and is a bit confusing since they are also OPTIONs (why are
> they called out when the others are not).  Might be simpler to just use:
> 
> usage: qemu-ga [OPTION]...
> 
> > +@c man end
> > +@end example
> > +
> > +@c man begin DESCRIPTION
> > +
> > +The QEMU Guest Agent is a daemon that allows the host to perform
> > +various operations in the guest, such as:
> 
> How about adding some words to make it obvious that it is not useful on
> bare metal and is intended for virtualized guests:
> 
> The QEMU Guest Agent is a daemon intended to be run in virtual machines.
>  It allows the hypervisor host to perform various operations in the
> guest, such as:
> 
> > +@c man begin OPTIONS
> > +@table @option
> > +@item -m, --method=@var{method}
> > +  Transport method: one of @samp{unix-listen}, @samp{virtio-serial}, or
> > +  @samp{isa-serial} (@samp{virtio-serial} is the default).
> > +
> > +@item -p, --path=@var{path}
> > +  Device/socket path (the default for virtio-serial is:
> > +  @samp{/dev/virtio-ports/org.qemu.guest_agent.0},
> > +  the default for isa-serial is: @samp{/dev/ttyS0})
> 
> s/is:/is/ in both places
> 
> > +
> > +@item -l, --logfile=@var{path}
> > +  Set log file path, logs to stderr by default.
> 
> Also, 'path' is an annoying term; POSIX folks use it for any file name,
> while GNU folks claim it should only be used for colon-separated lists
> of directories as in PATH=.  'name' or 'location' might be better,
> although I personally don't care strongly enough to reject on just that
> choice of naming.
> 
> Elsewhere you've listed defaults in (); maybe:
> 
> Set log file (default is stderr).
> 
> > +
> > +@item -f, --pidfile=@var{path}
> > +  Specify pid file (default is @samp{/var/run/qemu-ga.pid}).
> 
> Same bit about 'path', here and below (I'll quit pointing it out).
> 
> > +
> > +@item -F, --fsfreeze-hook=@var{path}
> > +  Enable fsfreeze hook. Accepts an optional argument that specifies
> > +  script to run on freeze/thaw. Script will be called with
> > +  'freeze'/'thaw' arguments accordingly.  (default is
> > +  @samp{/etc/qemu/fsfreeze-hook}) If using -F with an argument, do
> 
> Place the '.' after the (), not before, since the 'If using' sentence
> starts a new thought.
> 
> > +  not follow -F with a space. (for example:
> > +  @samp{-F/var/run/fsfreezehook.sh})
> 
> And again.
> 
> [aside: short options that take an optional argument are wonky; POSIX
> discourages their use in new programs, and GNU recommends that only long
> options have optional arguments. But we've already shipped it that way,
> so too late to change it now]
> 
> > +@item -b, --blacklist=@var{list}
> > +  Comma-separated list of RPCs to disable (no spaces, @samp{?} to list
> > +  available RPCs).
> > +
> 
> '?' requires shell quoting to avoid unintended globbing based on the
> contents of the current directory; in the past, we've updated 'qemu' to
> take 'help' as a synonym anywhere that '?' was special, so that you can
> query without worrying about quoting.  We should probably do likewise
> for the agent daemon; but as a separate patch.
> 
> Overall, fairly clean.

Thanks for the review. Since these are grammatical fixes I've gone ahead and
squashed these changes in, the only exception being suggestions regarding
'path' (the POSIX interpretation seems to make much more sense than associating
it with an unrelated environment variable's semantics), and the ones that are
outside the scope of this patch (short option args and 'help' as an alternative
to '?').

https://github.com/mdroth/qemu/commit/0e58c84d8618532bf6c3dcd3d13139fed5aaf3ca

Sending a pull later this week, can add your Reviewed-by if it looks okay.

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