From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v4 06/21] libxl: introduce libxl__vnuma_config_check Date: Thu, 29 Jan 2015 11:04:15 +0000 Message-ID: <1422529455.30641.37.camel@citrix.com> References: <1422011632-22018-1-git-send-email-wei.liu2@citrix.com> <1422011632-22018-7-git-send-email-wei.liu2@citrix.com> <1422461608.5187.41.camel@citrix.com> <20150128215131.GB14935@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150128215131.GB14935@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: ufimtseva@gmail.com, andrew.cooper3@citrix.com, dario.faggioli@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, JBeulich@suse.com List-Id: xen-devel@lists.xenproject.org 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.