Re: [Xen-devel] [PATCH v6 7/9] xl: enable getting and setting soft

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-05-28 at 01:42 +0100, Dario Faggioli wrote:
> @@ -4653,17 +4656,32 @@ int main_vcpulist(int argc, char **argv)
>      return 0;
>  }
>  
> -static int vcpupin(uint32_t domid, const char *vcpu, char *cpu)
> +int main_vcpupin(int argc, char **argv)
>  {
>      libxl_vcpuinfo *vcpuinfo;
> -    libxl_bitmap cpumap;
> -
> -    uint32_t vcpuid;
> +    libxl_bitmap cpumap, cpumap_soft;;
> +    uint32_t vcpuid, domid;
> +    const char *vcpu;
>      char *endptr;
> -    int i, nb_cpu, nb_vcpu, rc = -1;
> +    int opt, nb_cpu, nb_vcpu, rc = -1;
> +    libxl_bitmap *soft = &cpumap_soft, *hard = &cpumap;
> +    static struct option opts[] = {
> +        {"soft", 0, 0, 's'},

I think this is a remnant of a previous iteration?

> +        COMMON_LONG_OPTS,
> +        {0, 0, 0, 0}
> +    };
>  
>      libxl_bitmap_init(&cpumap);
> +    libxl_bitmap_init(&cpumap_soft);
>  
> +    SWITCH_FOREACH_OPT(opt, "", opts, "vcpu-pin", 3) {
> +        /* No options */
> +    }
> +
> +    domid = find_domain(argv[optind]);
> +    vcpu = argv[optind+1];
> +
> +    /* Figure out with which vCPU we are dealing with */
>      vcpuid = strtoul(vcpu, &endptr, 10);
>      if (vcpu == endptr) {
>          if (strcmp(vcpu, "all")) {
> @@ -4673,10 +4691,27 @@ static int vcpupin(uint32_t domid, const char *vcpu, 
> char *cpu)
>          vcpuid = -1;
>      }
>  
> -    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0))
> +    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0) ||
> +        (argc == 5 && libxl_cpu_bitmap_alloc(ctx, &cpumap_soft, 0)))

I think you could omit the argc==5 here, one measly allocation is all it
is saving you.

>          goto out;
>  
> -    if (vcpupin_parse(cpu, &cpumap))
> +    /*
> +     * Syntax is: xl vcpu-pin <domid> <vcpu> <hard> <soft>
> +     * We want to handle all the following cases ('-' means
> +     * "leave it alone"):
> +     *  xl vcpu-pin 0 3 3,4
> +     *  xl vcpu-pin 0 3 3,4 -
> +     *  xl vcpu-pin 0 3 - 6-9
> +     *  xl vcpu-pin 0 3 3,4 6-9
> +     */
> +    if (argc <= 4 || (argc == 5 && !strcmp(argv[optind+3], "-")))

You are mixing raw argc values and offsets from optind here, which is a
bit confusing.

It would be valid to do
        argc -= optind;
        argv += optind;
        optind = 0;
and then use argv[0] ...[1] etc.

Or just use optind+N for everything.

> +        soft = NULL;
> +    if (argc == 5 && !strcmp(argv[optind+2], "-"))
> +        hard = NULL;
> +
> +    if (hard && vcpupin_parse(argv[optind+2], &cpumap))
> +        goto out;
> +    if (soft && vcpupin_parse(argv[optind+3], &cpumap_soft))
>          goto out;

You've just parsed argv[option+2] and [...+3] into hard and soft
respectively.

Might some of this be simplified by making vcpupin_parse accept NULL
and/or making it handle "-"? e.g. 
        hard = argv[opting+2]
        soft = argc > XXX > argc[optind+3] : NULL
        vcpupin_parse(&hard, &cpumap);
        vcpupin_parse(&soft, &cpumap);

(sets hard/soft == NULL on input of "-", returns silently on hard/soft
== NULL.

> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index 4279b9f..d03c52a 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -218,7 +218,7 @@ struct cmd_spec cmd_table[] = {
>      { "vcpu-pin",
>        &main_vcpupin, 1, 1,
>        "Set which CPUs a VCPU can use",
> -      "<Domain> <VCPU|all> <CPUs|all>",
> +      "<Domain> <VCPU|all> <Hard affinity|all> <Soft affinity|all",

Missing a closing >

Also, I think "-" is a valid alternative now, right?

>      },
>      { "vcpu-set",
>        &main_vcpuset, 0, 1,



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel