Re: [PATCH v4 06/21] libxl: introduce libxl__vnuma_config_check

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2015-01-28 at 21:51 +0000, Wei Liu wrote:
> On Wed, Jan 28, 2015 at 04:13:28PM +0000, Ian Campbell wrote:
> > On Fri, 2015-01-23 at 11:13 +0000, Wei Liu wrote:
> > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > > index 6d3ac58..39356ba 100644
> > > --- a/tools/libxl/libxl_internal.h
> > > +++ b/tools/libxl/libxl_internal.h
> > > @@ -3394,6 +3394,11 @@ void libxl__numa_candidate_put_nodemap(libxl__gc *gc,
> > >      libxl_bitmap_copy(CTX, &cndt->nodemap, nodemap);
> > >  }
> > >  
> > > +/* Check if vNUMA config is valid. Returns 0 if valid. */
> > 
> > And on invalid? ERROR_FOO or just !0?
> > 
> > I see further down it is ERROR_INVAL, perhaps say so, and perhaps
> > introduce ERROR_INVAL_VNUMA_CONFIGUATION or something.
> 
> I think ERROR_INVALID is good enough. There are logs along the line to
> indicate what goes wrong.

The xapi guys have indicated that having to parse log lines is an issue
for them, and are planning to help clean this up.

We are supposed to have a policy of specific error codes which we've not
really done a good job of applying. I think we can start not adding new
uses of generic error codes. (ERROR_FAIL is the worst offender, but
ERROR_INVALID isn't much better IMHO).


> > > +    for (i = 0; i < b_info->num_vnuma_nodes; i++) {
> > > +        uint32_t pnode;
> > > +
> > > +        p = &b_info->vnuma_nodes[i];
> > > +        pnode = p->pnode;
> > > +
> > > +        /* The pnode specified is not valid? */
> > > +        if (pnode >= nr_nodes) {
> > 
> > I take it that pnodes aren't (potentially) sparse?
> > 
> 
> That is the assumption used in libxc and xen I think. I haven't seen
> sparse pnodes personally.

OK. The question is really whether nodes can be sparse in h/w and if so
does Xen hide that from us. I assume in h/w == yes and the hope here is
that Xen will hide it...


> > > +            goto out;
> > > +        }
> > > +
> > > +        total_memkb += p->memkb;
> > > +    }
> > > +
> > > +    if (total_memkb != b_info->max_memkb) {
> > > +        LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
> > > +                   "Amount of memory mismatch (0x%"PRIx64" != 0x%"PRIx64")",
> > > +                   total_memkb, b_info->max_memkb);
> > 
> > Did arch_setup_meminit not also check this?
> > 
> 
> Yes. But I think libxl should perform due diligence as well.  Logging
> here is also more user friendly.

OK

> > Meaning it could maybe abort() or ASSERT. Not 100% sure about that being
> > wise though.
> > 
> 
> No need to abort, because this function is used to validate user input.

I think I meant the libxc version -- which in theory should never see
invalid input because libxl has arranged never to pass it any... But
anyway, not sure that's a good idea.

Ian.