Re: [pve-devel] [PATCH cluster v10 1/5] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Tue, Nov 15, 2022 at 02:02:26PM +0100, Dominik Csapak wrote:
> for getting multiple properties from the in memory config of the
> guests in one go. Keep the existing IPC call as is for backward
> compatibility and add this as separate, new one.
> 
> It basically behaves the same as
> CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties
> instead and returns multiple properties per guest.
> 
> The old way of getting a single property is now also done by
> the new function, since it basically performs identically.
> 
> Here a short benchmark:
> 
> Setup:
> PVE in a VM with cpu type host (12700k) and 4 cores
> 10000 typical configs with both 'lock' and 'tags' set at the end
> and fairly long tags ('test-tag1,test-tag2,test-tag3')
> (normal vm with a snapshot, ~1KiB)
> 
> i let it run 100 times each, times in ms
> 
> old code:
> 
> total time  time per iteration
> 1054.2      10.2
> 
> new code:
> 
> num props  total time  time per iter  function
> 2          1332.2      13.2           get_properties
> 1          1051.2      10.2           get_properties
> 2          2082.2      20.2           get_property (2 calls to get_property)
> 1          1034.2      10.2           get_property
> 
> so a call with the new code for one property is the same as with the
> old code, and adding a second property only adds a bit of additional
> time (in this case ~30%)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---

Sort-of
Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

but I do have a bit of a maintainability-complaint and potential actual
issue (see below).

> changes from v9:
> * line_len, prop_len -> size_t
> * document min/max
> * remove setting of null byte in loop (it's already split at the null byte)
> * improved the loop for skipping whitespace at the beginning
>  data/src/cfs-ipc-ops.h |   2 +
>  data/src/server.c      |  63 ++++++++++++++
>  data/src/status.c      | 184 ++++++++++++++++++++++++++++-------------
>  data/src/status.h      |   3 +
>  4 files changed, 194 insertions(+), 58 deletions(-)
> 
> diff --git a/data/src/status.c b/data/src/status.c
> index 9bceaeb..47cb42e 100644
> --- a/data/src/status.c
> +++ b/data/src/status.c
> @@ -804,25 +804,55 @@ cfs_create_vmlist_msg(GString *str)
>  	return 0;
>  }
>  
> -// checks the conf for a line starting with '$prop:' and returns the value
> -// afterwards, whitout initial whitespace(s), we only deal with the format
> -// restricion imposed by our perl VM config parser, main reference is
> +// checks if a config line starts with the given prop. if yes, writes a '\0'
> +// at the end of the value, and returns the pointer where the value starts

Please add:

// `line[line_len]` is guaranteed to be `\0`

keep reading for why ;-)

> +char *
> +_get_property_value_from_line(char *line, size_t line_len, const char *prop, size_t prop_len)
> +{
> +	if (line_len <= prop_len + 1) return NULL;
> +
> +	if (line[prop_len] == ':' && memcmp(line, prop, prop_len) == 0) { // found
> +		char *v_start = &line[prop_len + 1];
> +		char *v_end = &line[line_len - 1];

nit: in most C code an start and end pointers are exclusive on the end side
(iow. end should point to the first invalid address) it's a bit
confusing doing it this way and may lead to confusion...

> +
> +		// drop initial value whitespaces here already
> +		while (v_end > v_start && *v_start && isspace(*v_start)) v_start++;

(((that's fine, but I find 'v_start < v_end' much easier to parse)))

> +
> +		if (!*v_start) return NULL;
> +

this is where the v_end tripped me up a bit:

- if there's no value, v_start == v_end and the loop won't run

> +		while (v_end > v_start && isspace(*v_end)) v_end--;
> +		v_end[1] = '\0';

which makes this read as "write to 1 past the end"

which *seems* fine at first given that we split the lines by replacing
'\n' with '\0', but then there's another case to think about:
The original data just came from a `memdb_read()` which just gives us a
"binary blob" and may not be newline-terminated.

However, the *caller* guarantees that in this case, we simply ignore the
final line.

BUT: do we do the same in perl?

> +
> +		return v_start;
> +	}
> +
> +	return NULL;
> +}
> +
> +// checks the conf for lines starting with the given props and
> +// writes the pointers into the correct positions into the 'found' array
> +// afterwards, without initial whitespace(s), we only deal with the format
> +// restriction imposed by our perl VM config parser, main reference is
>  // PVE::QemuServer::parse_vm_config this allows to be very fast and still
>  // relatively simple
> -// main restrictions used for our advantage is the properties match reges:
> +// main restrictions used for our advantage is the properties match regex:
>  // ($line =~ m/^([a-z][a-z_]*\d*):\s*(.+?)\s*$/) from parse_vm_config
>  // currently we only look at the current configuration in place, i.e., *no*
> -// snapshort and *no* pending changes
> -static char *
> -_get_property_value(char *conf, int conf_size, const char *prop, int prop_len)
> +// snapshot and *no* pending changes
> +//
> +// min..max is the char range of the first character of the given props,
> +// so that we can return early when checking the line
> +void
> +_get_property_values(char **found, char *conf, int conf_size, const char **props, uint8_t num_props, char min, char max)
>  {
>  	const char *const conf_end = conf + conf_size;
>  	char *line = conf;
> -	size_t remaining_size;
> +	size_t remaining_size = conf_size;
> +	int count = 0;

^ I'd prefer an uint8_t here and call it `prop_count` as it's strictly
to count towards the above `uint8_t num_props`, so `int` makes little
sense, and the generic name `count` with type `uint8_t` is also a bit
awkward ;-)

>  
>  	char *next_newline = memchr(conf, '\n', conf_size);
>  	if (next_newline == NULL) {
> -		return NULL; // valid property lines end with \n, but none in the config
> +		return; // valid property lines end with \n, but none in the config
>  	}
>  	*next_newline = '\0';
>  
> @@ -830,41 +860,32 @@ _get_property_value(char *conf, int conf_size, const char *prop, int prop_len)
>  		if (!line[0]) goto next;
>  
>  		// snapshot or pending section start, but nothing found yet -> not found
> -		if (line[0] == '[') return NULL;
> -		// properties start with /^[a-z]/, so continue early if not
> -		if (line[0] < 'a' || line[0] > 'z') goto next;
> -
> -		int line_len = strlen(line);
> -		if (line_len <= prop_len + 1) goto next;
> -
> -		if (line[prop_len] == ':' && memcmp(line, prop, prop_len) == 0) { // found
> -			char *v_start = &line[prop_len + 1];
> -
> -			// drop initial value whitespaces here already
> -			while (*v_start && isspace(*v_start)) v_start++;
> -
> -			if (!*v_start) return NULL;
> -
> -			char *v_end = &line[line_len - 1];
> -			while (v_end > v_start && isspace(*v_end)) v_end--;
> -			v_end[1] = '\0';
> -
> -			return v_start;
> +		if (line[0] == '[') return;
> +		// continue early if line does not begin with the min/max char of the properties
> +		if (line[0] < min || line[0] > max) goto next;
> +
> +		size_t line_len = strlen(line);

This is only fine because we skip the final line if it's not
`\n`-terminated (as it's otherwise not guaranteed to be `\0`-terminated)

But we don't even need to re-scan the line, since we already did that,
and should just be able to use:

    line_len = next_newline - line;

no?

> +		for (uint8_t i = 0; i < num_props; i++) {
> +			char * value = _get_property_value_from_line(line, line_len, props[i], strlen(props[i]));
> +			if (value != NULL) {
> +				count += (found[i] != NULL) & 0x1; // count newly found lines
> +				found[i] = value;
> +			}
> +		}
> +		if (count == num_props) {
> +			return; // found all
>  		}
>  next:
>  		line = next_newline + 1;
>  		remaining_size = conf_end - line;
> -		if (remaining_size <= prop_len) {
> -			return NULL;
> -		}
>  		next_newline = memchr(line, '\n', remaining_size);
>  		if (next_newline == NULL) {
> -			return NULL; // valid property lines end with \n, but none in the config
> +			return; // valid property lines end with \n, but none in the config
>  		}
>  		*next_newline = '\0';
>  	}
>  
> -	return NULL; // not found
> +	return;
>  }
>  
>  static void