From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions Date: Thu, 16 Jul 2015 11:30:00 +0200 Message-ID: <55A77998.9090307@suse.com> References: <1436892073-14186-1-git-send-email-wei.liu2@citrix.com> <1436892073-14186-11-git-send-email-wei.liu2@citrix.com> <21925.18402.35455.636018@mariner.uk.xensource.com> <20150715171643.GI12455@zion.uk.xensource.com> <1437037044.13522.219.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZFfUS-0002HK-To for xen-devel@lists.xenproject.org; Thu, 16 Jul 2015 09:30:05 +0000 In-Reply-To: <1437037044.13522.219.camel@citrix.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: Dario Faggioli , Wei Liu Cc: Xen-devel , Ian Jackson , Ian Campbell List-Id: xen-devel@lists.xenproject.org On 07/16/2015 10:57 AM, Dario Faggioli wrote: > [Adding Juergen] Near miss. You took my old mail address. :-) > > On Wed, 2015-07-15 at 18:16 +0100, Wei Liu wrote: >> On Tue, Jul 14, 2015 at 06:33:22PM +0100, Ian Jackson wrote: >>> Wei Liu writes ("[PATCH v3 10/10] libxl: fix caller of libxl_cpupool functions"): > >>>> --- a/tools/libxl/libxl.c >>>> +++ b/tools/libxl/libxl.c >>>> @@ -771,8 +771,11 @@ libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool_out) >>>> >>>> poolid = 0; >>>> for (i = 0;; i++) { >>>> - if (cpupool_info(gc, &info, poolid, false)) >>>> + libxl_cpupoolinfo_init(&info); >>>> + if (cpupool_info(gc, &info, poolid, false)) { >>>> + libxl_cpupoolinfo_dispose(&info); >>>> break; >>>> + } >>> >>> I'm not convinced by this change. >>> >>> I think that this function has broken error handling: if cpupool_info >>> fails, it simply ignores the error. >>> >> >> I think the semantics of this function is to get back as many cpupool >> info as it can. >> > Yes, I also think this is the case. Indeed. >> Unfortunately the failing of cpupool_info can either mean the cpupool id >> does not exist or other internal errors. So not cleaning up is the >> right thing to do (speaking from semantics point of view). >> > Indeed. > >> I think I need to overhaul cpupool_info a bit if we want to make this >> API better. >> > Well, perhaps having cpupool_info() treating specially the situation > where the pool ID is not there may help... However, what would you do > once you have this additional piece of information available? > > Maybe, depending on the error, we can cleanup the whole array? Is it > this that we are after? I think the best would be: Modify cpupool_info() to return either success, internal error, or no cpupool found. In case of internal error libxl_list_cpupool() should clean up and return NULL. > >> I'm not sure if it is possible to interleave pool ids. If so, we need a >> way to get back the exact number of cpupools. Dario? libxl_list_cpupool() does that (2nd parameter). >> > Sorry, what you mean by 'interleave pool ids'? Not sure if this is an answer to interleaving of pool ids, but it is possible to specify the pool id when creating a new cpupool at the libxc interface. Even in case this is not used, pool ids can easily be sparse after having deleted a cpupool. Juergen