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