Re: [RFC PATCH 2/2] tools/misc: Add xen-stats tool

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

Hi Matias,

On Tue, May 17, 2022 at 04:33:15PM +0200, Matias Ezequiel Vara Larsen wrote:
> Add a demostration tool that uses the stats_table resource to
> query vcpu time for a DomU.
> 
> Signed-off-by: Matias Ezequiel Vara Larsen <matias.vara@xxxxxxxx>
> ---
> diff --git a/tools/misc/Makefile b/tools/misc/Makefile
> index 2b683819d4..b510e3aceb 100644
> --- a/tools/misc/Makefile
> +++ b/tools/misc/Makefile
> @@ -135,4 +135,9 @@ xencov: xencov.o
>  xen-ucode: xen-ucode.o
>       $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
>  
> +xen-stats.o: CFLAGS += $(CFLAGS_libxenforeginmemory)
> +
> +xen-stats: xen-stats.o

The tools seems to be only about vcpu, maybe `xen-stats` is a bit too
generic. Would `xen-vcpus-stats`, or maybe something with `time` would
be better?

Also, is it a tools that could be useful enough to be installed by
default? Should we at least build it by default so it doesn't rot? (By
adding it to only $(TARGETS).)

> +     $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) 
> $(LDLIBS_libxenforeignmemory) $(APPEND_LDFLAGS)
> +
>  -include $(DEPS_INCLUDE)
> diff --git a/tools/misc/xen-stats.c b/tools/misc/xen-stats.c
> new file mode 100644
> index 0000000000..5d4a3239cc
> --- /dev/null
> +++ b/tools/misc/xen-stats.c
> @@ -0,0 +1,83 @@
> +#include <err.h>
> +#include <errno.h>
> +#include <error.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <signal.h>
> +
> +#include <xenctrl.h>

It seems overkill to use this header when the tool only use
xenforeignmemory interface. But I don't know how to replace
XC_PAGE_SHIFT, so I guess that's ok.

> +#include <xenforeignmemory.h>
> +#include <xen-tools/libs.h>

What do you use this headers for? Is it left over?

> +static sig_atomic_t interrupted;
> +static void close_handler(int signum)
> +{
> +    interrupted = 1;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    xenforeignmemory_handle *fh;
> +    xenforeignmemory_resource_handle *res;
> +    size_t size;
> +    int rc, nr_frames, domid, frec, vcpu;
> +    uint64_t * info;
> +    struct sigaction act;
> +
> +    if (argc != 4 ) {
> +        fprintf(stderr, "Usage: %s <domid> <vcpu> <period>\n", argv[0]);
> +        return 1;
> +    }
> +
> +    // TODO: this depends on the resource
> +    nr_frames = 1;
> +
> +    domid = atoi(argv[1]);
> +    frec = atoi(argv[3]);
> +    vcpu = atoi(argv[2]);

Can you swap the last two line? I think it is better if the order is the same
as on the command line.

> +
> +    act.sa_handler = close_handler;
> +    act.sa_flags = 0;
> +    sigemptyset(&act.sa_mask);
> +    sigaction(SIGHUP,  &act, NULL);
> +    sigaction(SIGTERM, &act, NULL);
> +    sigaction(SIGINT,  &act, NULL);
> +    sigaction(SIGALRM, &act, NULL);
> +
> +    fh = xenforeignmemory_open(NULL, 0);
> +
> +    if ( !fh )
> +        err(1, "xenforeignmemory_open");
> +
> +    rc = xenforeignmemory_resource_size(
> +        fh, domid, XENMEM_resource_stats_table,
> +        vcpu, &size);
> +
> +    if ( rc )
> +        err(1, "    Fail: Get size: %d - %s\n", errno, strerror(errno));

It seems that err() already does print strerror(), and add a "\n", so
why print it again? Also, if we have strerror(), what the point of
printing "errno"?

Also, I'm not sure the extra indentation in the error message is really
useful, but that doesn't really matter.

> +
> +    if ( (size >> XC_PAGE_SHIFT) != nr_frames )
> +        err(1, "    Fail: Get size: expected %u frames, got %zu\n",
> +                    nr_frames, size >> XC_PAGE_SHIFT);

err() prints strerror(errno), maybe errx() is better here.


Thanks,

-- 
Anthony PERARD