On Wed, 2017-01-18 at 10:02 +0800, Yi Sun wrote:
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -9523,19 +9523,21 @@ int main_psr_cat_cbm_set(int argc, char
> **argv)
>      char *value;
>      libxl_string_list socket_list;
>      unsigned long start, end;
> -    int i, j, len;
> +    unsigned int i, j, len;
>
This type change has nothing to do with this very patch, has it?

> +    unsigned int lvl = 3;
>  
Yeah, well, I understand that it's an improvement, and that, while
you're making changes in the area, you may want to do it... but at the
same time it is undeniably confusing to see the change here.

Personally, I don't like it to be part of this patch, but this is not
my call. :-)

> @@ -9555,17 +9557,24 @@ int main_psr_cat_cbm_set(int argc, char
> **argv)
>      case 'c':
>          opt_code = 1;
>          break;
> +    case 'l':
> +        lvl = atoi(optarg);
> +        break;
>      }
>  
> -    if (opt_data && opt_code) {
> -        fprintf(stderr, "Cannot handle -c and -d at the same
> time\n");
> -        return -1;
> -    } else if (opt_data) {
> -        type = LIBXL_PSR_CBM_TYPE_L3_CBM_DATA;
> -    } else if (opt_code) {
> -        type = LIBXL_PSR_CBM_TYPE_L3_CBM_CODE;
> -    } else {
> -        type = LIBXL_PSR_CBM_TYPE_L3_CBM;
> +    if (lvl == 2)
> +        type = LIBXL_PSR_CBM_TYPE_L2_CBM;
> +    else if (lvl == 3) {
> +        if (opt_data && opt_code) {
> +            fprintf(stderr, "Cannot handle -c and -d at the same
> time\n");
> +            return ERROR_FAIL;
>
When the xl program is terminating --and in this case it is, as we are
returning from a main_foo_bar() function-- the exit code should be
either EXIT_SUCCESS (if everything went ok) or EXIT_FAILURE (if
something went wrong, like in this case).

I know this not yet happens everywhere in xl, but, please, let's cope
with that for new code.

Apart of these two things I've mentioned, the patch looks ok to me.

Regards,
Dario

> +        } else if (opt_data) {
> +            type = LIBXL_PSR_CBM_TYPE_L3_CBM_DATA;
> +        } else if (opt_code) {
> +            type = LIBXL_PSR_CBM_TYPE_L3_CBM_CODE;
> +        } else {
> +            type = LIBXL_PSR_CBM_TYPE_L3_CBM;
> +        }
>      }
>  
>      if (libxl_bitmap_is_empty(&target_map))
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index c5fbad4..32c3ee5 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -550,6 +550,7 @@ struct cmd_spec cmd_table[] = {
>        "Set cache capacity bitmasks(CBM) for a domain",
>        "[options] <Domain> <CBM>",
>        "-s <socket>       Specify the socket to process, otherwise
> all sockets are processed\n"
> +      "-l <level>        Specify the cache level to process,
> otherwise L3 cache is processed\n"
>        "-c                Set code CBM if CDP is supported\n"
>        "-d                Set data CBM if CDP is supported\n"
>      },
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)