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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.

> 
> > +/* Check if vNUMA configuration is valid:
> > + *  1. all pnodes inside vnode_to_pnode array are valid
> > + *  2. one vcpu belongs to and only belongs to one vnode
> > + *  3. each vmemrange is valid and doesn't overlap with each other
> > + */
> > +int libxl__vnuma_config_check(libxl__gc *gc,
> > +                         const libxl_domain_build_info *b_info,
> 
> Hard tabs here.
> 

Fixed.

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

> > +            LIBXL__LOG(CTX, LIBXL__LOG_ERROR,
> > +                       "Invalid pnode %d specified",
> > +                       pnode);
> 
> Please use the short LOG macros (throughout).
> 

Ack.

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

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

Wei.

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