Re: [PATCH v4 3/4] tools: add tools support for Intel CDP

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

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.