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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3904 bytes --]

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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]