[pve-devel] [PATCH cluster v10 1/5] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method
Wolfgang Bumiller
w.bumiller at proxmox.com
Wed Nov 16 10:50:38 CET 2022
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 at proxmox.com>
> ---
Sort-of
Acked-by: Wolfgang Bumiller <w.bumiller at 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
More information about the pve-devel
mailing list