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

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3449 bytes --]

On Wed, 2014-05-28 at 16:33 +0100, Ian Campbell wrote:
> 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?
> 
Gah! Sorry for this. I looked at the whole series looking for remnant a
few times, but I guess it's true that we're blind wrt our own code!

> > -    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.
> 
I agree it's confusing, and I like this solution above, I'll go for it.

> > +        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.
> 
I think this will make it simpler. I'll give it a try.

> > 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 >
> 
Ok.

> Also, I think "-" is a valid alternative now, right?
> 
Yes it is.

Thanks and Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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