Re: [PATCH 1/2 v2] node_memory: Do not fail if there is parameter unsupported

[Eric Blake <eblake@xxxxxxxxxx>]

> It makes no sense to fail the whole getting command if there is
> parameter unsupported by the kernel. This patch fixes it by

s/parameter/a parameter/

> omitting the unsupported parameter for getMemoryParameters.
> 
> And For setMemoryParameters, this checks if there is unsupported

s/For/for/
s/is/is an/

> parameter up front of the setting, and just returns failure if not
> all parameters are supported.
> 

> - * filled with parameter information, which might be less but will
> - * not exceed the input value.
> + * Get all node memory parameters (parameter unsupported by OS will
> be

s/parameter/parameters/

>  #ifdef __linux__
>  static int
> -nodeSetMemoryParameterValue(const char *field,
> -                            virTypedParameterPtr param)
> +nodeSetMemoryParameterValue(virTypedParameterPtr param)
>  {
>      char *path = NULL;
>      char *strval = NULL;
>      int ret = -1;
>      int rc = -1;
>  
> +    char *field = strchr(param->field, '_');
> +    field++;

This site should be safe (we only get here if we got past earlier
filters), but...

> +static bool
> +nodeMemoryParametersIsAllSupported(virTypedParameterPtr params,
> +                                   int nparams)
> +{
> +    char *path = NULL;
> +    int i;
> +
> +    for (i = 0; i < nparams; i++) {
> +        virTypedParameterPtr param = &params[i];
> +
> +        char *field = strchr(param->field, '_');
> +        field++;

Are we guaranteed that field is non-NULL, or could the user pass us
a param->field name that doesn't contain '_', in which case we are
doing bad dereferencing?  That is, since this is the earlier filter
that makes the rest of the code safe, I think you need to be prepared
for the user to pass unexpected strings, and fail if field is NULL
at this point.

ACK with that fixed.

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list