Re: [PATCH] examples: Introduce domtop

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07/16/2014 07:53 AM, Michal Privoznik wrote:
> There's this question on the list that is asked over and over again.
> How do I get {cpu, memory, ...} usage in percentage? Or its modified
> version: How do I plot nice graphs like virt-manager does?
> 
> It would be nice if we have an example to inspire people. And that's
> what domtop should do. Yes, it could be written in different ways, but
> I've chosen this one as I think it show explicitly what users need to
> implement in order to imitate virt-manager's graphing.
> 
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
>  .gitignore                  |   1 +
>  Makefile.am                 |   2 +-
>  cfg.mk                      |   2 +-
>  configure.ac                |   1 +
>  examples/domtop/Makefile.am |  27 +++
>  examples/domtop/domtop.c    | 388 ++++++++++++++++++++++++++++++++++++++++++++
>  libvirt.spec.in             |   2 +-
>  7 files changed, 420 insertions(+), 3 deletions(-)
>  create mode 100644 examples/domtop/Makefile.am
>  create mode 100644 examples/domtop/domtop.c
> 

[first round review - I still plan to compile and examine what happens
when actually running the program, which may result in more comments...]

> +++ b/cfg.mk
> @@ -1078,7 +1078,7 @@ exclude_file_name_regexp--sc_prohibit_sprintf = \
>  exclude_file_name_regexp--sc_prohibit_strncpy = ^src/util/virstring\.c$$
>  
>  exclude_file_name_regexp--sc_prohibit_strtol = \
> -  ^(src/(util/virsexpr|(vbox|xen|xenxs)/.*)\.c)|(examples/domsuspend/suspend.c)$$
> +  ^(src/(util/virsexpr|(vbox|xen|xenxs)/.*)\.c)|(examples/domsuspend/suspend.c)|(examples/domtop/domtop.c)$$

Long line.  I'd be happy with the shorter equivalent:

  ^(src/(util/virsexpr|(vbox|xen|xenxs)/.*)|examples/dom.*/.*)\.c$$

[side question - why are we allowing strtol in vbox, xen, and xenxs?
Probably worth an independent cleanup there]


> +++ b/examples/domtop/domtop.c
> @@ -0,0 +1,388 @@

> +
> +static int debug;
> +static int run_top = 1;

Worth including <stdbool.h> and making these bool?

> +
> +    printf("\n%s [options] [domain name]\n\n"
> +           "  options:\n"
> +           "    -d | --debug        enable debug printings\n"

s/printings/messages/

> +           "    -h | --help         print this help\n"
> +           "    -c | --connect=URI  hypervisor connection URI\n"
> +           "    -D | --delay=X      delay between updates in miliseconds\n",

s/miliseconds/milliseconds/

> +           unified_progname);
> +}
> +
> +static int
> +parse_argv(int argc, char *argv[],
> +           const char **uri,
> +           const char **dom_name,
> +           unsigned int *mili_seconds)

s/mili_seconds/milliseconds/


> +
> +    while ((arg = getopt_long(argc, argv, "+:dhc:D:", opt, NULL)) != -1) {
> +        switch (arg) {
> +        case 'd':
> +            debug = 1;

again, bool might be nicer here.

> +
> +    printf("Running domains:\n");
> +    printf("----------------\n");

Personal preference - I like puts() rather than printf() when there is
no % in the format string.


> +
> +static int
> +do_top(virConnectPtr conn,
> +       const char *dom_name,
> +       unsigned int mili_seconds)

s/mili_seconds/milliseconds/

Overall looks fairly useful.

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

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]