Re: [PATCH v2] examples: Introduce domtop

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

 



On 07/18/2014 03:08 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.
> 
> Note: The usage is displayed from host perspective. That is, how much
> host CPUs the domain is using. But it should be fairly simple to
> switch do just guest CPU usage if needed.
> 
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---

> +++ b/examples/domtop/domtop.c

> +
> +#define STREQ(a,b) (strcmp(a,b) == 0)

Space after comma, twice.

> +
> +static void
> +print_usage(const char *progname)
> +{
> +    const char *unified_progname;
> +
> +    if (!(unified_progname = strrchr(progname, '/')))
> +        unified_progname = progname;
> +    else
> +        unified_progname++;
> +
> +    printf("\n%s [options] [domain name]\n\n"
> +           "  options:\n"
> +           "    -d | --debug        enable debug messages\n"
> +           "    -h | --help         print this help\n"
> +           "    -c | --connect=URI  hypervisor connection URI\n"
> +           "    -D | --delay=X      delay between updates in milliseconds\n"

Maybe mention the default, if -D is not specified.

> +           "\n"
> +           "Print the cumulative usage of each host CPU. \n"
> +           "Without any domain name specified the list of \n"

Two instances of trailing whitespace in the output. s/ \n/\n/


> +        case 'D':
> +            /* strtoul man page suggest clearing errno prior to call */

s/suggest/suggests/

> +            errno = 0;
> +            val = strtoul(optarg, &p, 10);
> +            if (errno || *p || p == optarg) {
> +                ERROR("Invalid number: '%s'", optarg);
> +                goto cleanup;
> +            }
> +            *milliseconds = val;
> +            if (*milliseconds != val) {
> +                ERROR("Integer overflow: %ld", val);
> +                goto cleanup;

Looks odd to have 'goto cleanup' here...

> +            }
> +            break;
> +        case ':':
> +            ERROR("option '-%c' requires an argument", optopt);
> +            exit(EXIT_FAILURE);
> +        case '?':
> +            if (optopt)
> +                ERROR("unsupported option '-%c'. See --help.", optopt);
> +            else
> +                ERROR("unsupported option '%s'. See --help.", argv[optind - 1]);
> +            exit(EXIT_FAILURE);

...but direct exit here...

> +        default:
> +            ERROR("unknown option");
> +            exit(EXIT_FAILURE);
> +        }
> +    }
> +
> +    if (argc > optind)
> +        *dom_name = argv[optind];
> +
> +    ret = 0;
> + cleanup:
> +    return ret;

...especially since there is nothing cleaned up, and the main() function
eventually just exits anyway.  I don't care whether you keep the cleanup
label and consistently use it, or whether you just use direct exit()
everywhere in this function, but at least be consistent :)


> +static void
> +print_cpu_usage(const char *dom_name,
> +                size_t cpu,
> +                size_t ncpus,
> +                unsigned long long then,
> +                virTypedParameterPtr then_params,
> +                size_t then_nparams,
> +                unsigned long long now,
> +                virTypedParameterPtr now_params,
> +                size_t now_nparams)
> +{
> +    size_t i, j, k;
> +    size_t nparams = now_nparams;
> +    bool delim = false;
> +

> +    for (i = 0; i < ncpus; i++) {

> +
> +        if (delim)
> +            printf("\t");
> +        printf("CPU%zu: %.2lf", cpu + i, usage);
> +        delim = true;
> +    }
> +
> +    if (delim)
> +        printf("\n");

This 'if' is dead (it can only be false if ncpus is 0 - but all hosts
have at least one), so you can unconditionally print the \n.

ACK with nits fixed.

-- 
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]