Re: [Xen-devel] [PATCH for-4.6 v2 2/3] xl: error out if vNUMA specifies more vcpus than pcpus

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2015-08-14 at 00:38 +0100, Wei Liu wrote:
> On Fri, Aug 14, 2015 at 01:25:45AM +0200, Dario Faggioli wrote:

> > > --- a/tools/libxl/xl_cmdimpl.c
> > > +++ b/tools/libxl/xl_cmdimpl.c
> > > @@ -1202,11 +1202,27 @@ static void parse_vnuma_config(const XLU_Config 
> > > *config,
> > >      }
> > >  
> > >      /* User has specified maxvcpus= */
> > > -    if (b_info->max_vcpus != 0 &&  b_info->max_vcpus != max_vcpus) {
> > > -        fprintf(stderr, "xl: vnuma vcpus and maxvcpus= mismatch\n");
> > > -        exit(1);
> > > -    } else
> > > +    if (b_info->max_vcpus != 0) {
> > > +        if (b_info->max_vcpus != max_vcpus) {
> > > +            fprintf(stderr, "xl: vnuma vcpus and maxvcpus= mismatch\n");
> > > +            exit(1);
> > > +        }
> > > +    } else {
> > > +        int host_cpus = libxl_get_online_cpus(ctx);
> > > +
> > > +        if (host_cpus < 0) {
> > > +            fprintf(stderr, "Failed to get online cpus\n");
> > > +            exit(1);
> > > +        }
> > > +
> > > +        if (host_cpus < max_vcpus) {
> > > +            fprintf(stderr, "xl: vnuma specifies more vcpus than pcpus, 
> > > "\
> > > +                    "use maxvcpus= to override this check.\n");
> > >
> > ...isn't it too late, when we get to here?
> > 

> That's fine because that function has no effect when you try to set a
> bit beyond its size.
> 
Well, right. Fair enough.

> > Assuming I'm not, it seems to me that a solution could be to check for
> > this situation _inside_ the 'else if (!strcmp("vcpus", option))'. In
> > fact, if "maxvcpus" has not been specified, as soon as the end of one of
> > the ranges --as returned by parse_range()-- is beyond host_cpus, we know
> > we'd be going past the limit of the corresponding element of
> > vcpu_parsed, and we can error out.
> > 
> > It'll most likely be a bit uglier than this patch, but probably still
> > less complex than v1. :-)
> > 
> 
> That doesn't make any difference in terms of functionality. I would
> rather leave the parsing bit as it is and deal with fallout separately.
> That would make code cleaner IMHO.
> 
It's certainly cleaner as it is right now, in this version of the patch.

I still find the thing above a bit confusing, although correct because
of the fact that bitmap set is a nop if performed out of range.

Perhaps a short comment? Anyway, I don't have a strong opinion on this
so, with or without it (the comment):

Reviewed-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>

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: signature.asc
Description: This is a digitally signed message part

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