From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v4 3/4] tools: add tools support for Intel CDP Date: Fri, 25 Sep 2015 11:30:08 +0100 Message-ID: <1443177008.25250.112.camel@citrix.com> References: <1442482536-12024-1-git-send-email-he.chen@linux.intel.com> <1442482536-12024-4-git-send-email-he.chen@linux.intel.com> <1443092847.10338.283.camel@citrix.com> <20150925084359.GA12290@HE> <1443172735.25250.79.camel@citrix.com> <20150925095337.GD12290@HE> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZfQGb-00081q-5y for xen-devel@lists.xenproject.org; Fri, 25 Sep 2015 10:30:13 +0000 In-Reply-To: <20150925095337.GD12290@HE> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: He Chen Cc: wei.liu2@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, jbeulich@suse.com, xen-devel@lists.xenproject.org, chao.p.peng@linux.intel.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On Fri, 2015-09-25 at 17:53 +0800, He Chen wrote: > > Quoting the relevant bits of code for clarity: > > libxl_psr_cbm_type type = LIBXL_PSR_CBM_TYPE_L3_CBM; > > ... > > case 'd': > > type = LIBXL_PSR_CBM_TYPE_L3_DATA; > > opt_data = 1; > > break; > > case 'c': > > type = LIBXL_PSR_CBM_TYPE_L3_CODE; > > opt_code = 1; > > break; > > } > > > > if (opt_data && opt_code) > > type = LIBXL_PSR_CBM_TYPE_L3_CBM; > > > > So the behaviour if -d and -c are given is exactly the same as if > > neither > > of them were given, i.e. type = LIBXL_PSR_CBM_TYPE_L3_CBM? Is that > > correct > > and intended? > > Yes. > > > If so then I think it would be clearer to only set opt_* during option > > parsing and then to figure out the correct LIBXL_PSR_CBM_TYPE_* > > explicitly > > afterwards, rather than have the code cycle through data->code->cbm. > > > > Or just outlaw passing both -d and -c together since it is confusing > > and > > equivalent to passing neither anyway. > > Yes, as you said, if user just passes one option -d (or -c), things would > be done during option parsing, there is no need to add the if(). > > But the key point is that I am not sure how to address outlaw passing > both > -d and -c together (is it allowed?). If it is permitted, the behaviour is > the same as passing neither indeed, and the if() is needed to avoid > latter > option overwritting former option. > > What's your suggestion? Sorry, I am a little confused. > Omit former opiton when both options are given and remove if()? > Or something else? I was trying to make one suggestion for restructuring the code and one design choice to make, let me see if I can clarify. I think the basic code structure should be: libxl_psr_cbm_type type; int opt_data = 0, opt_code = 1; [...] case 'd': opt_data = 1; break; case 'c': opt_code = 1; break; [...] [... now figure out correct type= based on opt_data + opt+code... ] Which separates the option parsing from the logic of what they mean. Then the choice I mentioned is whether passing -c and -d at the same time is valid or not. If you want passing both -c and -d at the same time to be invalid then the code would be something like: if (opt_data && opt_code) { log error and exit } else if (opt_data) { type = LIBXL_PSR_CBM_TYPE_L3_DATA; } else if (opt_code) { type = LIBXL_PSR_CBM_TYPE_L3_CODE; else { /* Neither */ type = LIBXL_PSR_CBM_TYPE_L3_CBM; } If you want passing both -c and -d to be valid and behave like passing neither then it would be something like: if (opt_data && opt_code) { type = LIBXL_PSR_CBM_TYPE_L3_CBM; } else if (opt_data) { type = LIBXL_PSR_CBM_TYPE_L3_DATA; } else if (opt_code) { type = LIBXL_PSR_CBM_TYPE_L3_CODE; else { /* Neither, same as both */ type = LIBXL_PSR_CBM_TYPE_L3_CBM; } Which one you use is up to you, depending on what you think the most sensible semantics are. Ian.