[pve-devel] [PATCH cluster v9 1/4] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method
Wolfgang Bumiller
w.bumiller at proxmox.com
Mon Nov 14 14:15:40 CET 2022
On Mon, Nov 14, 2022 at 10:43:45AM +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>
> ---
> data/src/cfs-ipc-ops.h | 2 +
> data/src/server.c | 64 +++++++++++++++
> data/src/status.c | 177 ++++++++++++++++++++++++++++-------------
> data/src/status.h | 3 +
> 4 files changed, 190 insertions(+), 56 deletions(-)
>
> diff --git a/data/src/cfs-ipc-ops.h b/data/src/cfs-ipc-ops.h
> index 003e233..249308d 100644
> --- a/data/src/cfs-ipc-ops.h
> +++ b/data/src/cfs-ipc-ops.h
> @@ -43,4 +43,6 @@
>
> #define CFS_IPC_VERIFY_TOKEN 12
>
> +#define CFS_IPC_GET_GUEST_CONFIG_PROPERTIES 13
> +
> #endif
> diff --git a/data/src/server.c b/data/src/server.c
> index 549788a..ced9cfc 100644
> --- a/data/src/server.c
> +++ b/data/src/server.c
> @@ -89,6 +89,13 @@ typedef struct {
> char property[];
> } cfs_guest_config_propery_get_request_header_t;
>
> +typedef struct {
> + struct qb_ipc_request_header req_header;
> + uint32_t vmid;
> + uint8_t num_props;
> + char props[]; /* list of \0 terminated properties */
> +} cfs_guest_config_properties_get_request_header_t;
> +
> typedef struct {
> struct qb_ipc_request_header req_header;
> char token[];
> @@ -348,6 +355,63 @@ static int32_t s1_msg_process_fn(
>
> result = cfs_create_guest_conf_property_msg(outbuf, memdb, rh->property, rh->vmid);
> }
> + } else if (request_id == CFS_IPC_GET_GUEST_CONFIG_PROPERTIES) {
> +
> + cfs_guest_config_properties_get_request_header_t *rh =
> + (cfs_guest_config_properties_get_request_header_t *) data;
> +
> + size_t remaining = request_size - G_STRUCT_OFFSET(cfs_guest_config_properties_get_request_header_t, props);
> +
> + result = 0;
> + if (rh->vmid < 100 && rh->vmid != 0) {
> + cfs_debug("vmid out of range %u", rh->vmid);
> + result = -EINVAL;
> + } else if (rh->vmid >= 100 && !vmlist_vm_exists(rh->vmid)) {
> + result = -ENOENT;
> + } else if (rh->num_props == 0) {
> + cfs_debug("num_props == 0");
> + result = -EINVAL;
> + } else if (remaining <= 1) {
> + cfs_debug("property length <= 1, %ld", remaining);
> + result = -EINVAL;
> + } else {
> + const char **properties = malloc(sizeof(char*) * rh->num_props);
> + char *current = (rh->props);
> + for (uint8_t i = 0; i < rh->num_props; i++) {
> + size_t proplen = strnlen(current, remaining);
> + if (proplen == 0) {
> + cfs_debug("property length 0");
> + result = -EINVAL;
> + break;
> + }
> + if (proplen == remaining) {
> + cfs_debug("property not \\0 terminated");
> + result = -EINVAL;
> + break;
> + }
> + if (current[0] < 'a' || current[0] > 'z') {
> + cfs_debug("property does not start with [a-z]");
> + result = -EINVAL;
> + break;
> + }
> + properties[i] = current;
> + current[proplen] = '\0'; // ensure property is 0 terminated
^ this is useless since you're literally splitting at \0. If you feel
unsure, include `|| current[proplen]` in the `proplen == remaining`
check.
> + remaining -= (proplen + 1);
> + current += proplen + 1;
> + }
> +
> + if (remaining != 0) {
> + cfs_debug("leftover data after parsing %u properties", rh->num_props);
> + result = -EINVAL;
> + }
> +
> + if (result == 0) {
> + cfs_debug("cfs_get_guest_config_properties: basic valid checked, do request");
*validity
> + result = cfs_create_guest_conf_properties_msg(outbuf, memdb, properties, rh->num_props, rh->vmid);
> + }
> +
> + free(properties);
> + }
> } else if (request_id == CFS_IPC_VERIFY_TOKEN) {
>
> cfs_verify_token_request_header_t *rh = (cfs_verify_token_request_header_t *) data;
> diff --git a/data/src/status.c b/data/src/status.c
> index 9bceaeb..e2bedaa 100644
> --- a/data/src/status.c
> +++ b/data/src/status.c
> @@ -804,25 +804,52 @@ 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
> +char *
> +_get_property_value_from_line(char *line, int line_len, const char *prop, int prop_len)
line_len and prop_len should be size_t
and yes, that change may lead to fixing up currently-wrong types and
casting those pesky wrong high-level fuse integer types...
(`int read`, that's just painful)
> +{
> + 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];
> +
> + // drop initial value whitespaces here already
> + while (*v_start && isspace(*v_start)) v_start++;
^ this on its own might dereference bytes after the line
add a `line_end = line + line_len` and start with `v_start != line_end &&`.
> +
> + if (!*v_start) return NULL;
and then
v_start == line_end || !*v_start
> +
> + char *v_end = &line[line_len - 1];
> + while (v_end > v_start && isspace(*v_end)) v_end--;
> + v_end[1] = '\0';
> +
> + 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
> +void
> +_get_property_values(char **found, char *conf, int conf_size, const char **props, uint8_t num_props, char min, char max)
^ min/max should be documented, is it even really worth it?
> {
> const char *const conf_end = conf + conf_size;
> char *line = conf;
> - size_t remaining_size;
> + size_t remaining_size = conf_size;
> + int count = 0;
>
> 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 +857,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;
> + 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;
>
> 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;
> + 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
> @@ -883,24 +901,77 @@ _g_str_append_kv_jsonescaped(GString *str, const char *k, const char *v)
> }
>
> int
> -cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *prop, uint32_t vmid)
> +_print_found_properties(
> + GString *str,
> + gpointer conf,
> + int size,
> + const char **props,
> + uint8_t num_props,
> + uint32_t vmid,
> + char **values,
> + char min,
> + char max,
> + int first)
> +{
> + _get_property_values(values, conf, size, props, num_props, min, max);
> +
> + uint8_t found = 0;
> + for (uint8_t i = 0; i < num_props; i++) {
> + if (values[i] == NULL) {
> + continue;
> + }
> + if (found) {
> + g_string_append_c(str, ',');
> + } else {
> + if (!first) {
> + g_string_append_printf(str, ",\n");
> + } else {
> + first = 0;
> + }
> + g_string_append_printf(str, "\"%u\":{", vmid);
> + found = 1;
> + }
> + _g_str_append_kv_jsonescaped(str, props[i], values[i]);
> + }
> +
> + if (found) {
> + g_string_append_c(str, '}');
> + }
> +
> + return first;
> +}
> +
> +int
> +cfs_create_guest_conf_properties_msg(GString *str, memdb_t *memdb, const char **props, uint8_t num_props, uint32_t vmid)
> {
> g_return_val_if_fail(cfs_status.vmlist != NULL, -EINVAL);
> g_return_val_if_fail(str != NULL, -EINVAL);
>
> - int prop_len = strlen(prop);
> - int res = 0;
> - GString *path = NULL;
> -
> // Prelock &memdb->mutex in order to enable the usage of memdb_read_nolock
> // to prevent Deadlocks as in #2553
> g_mutex_lock (&memdb->mutex);
> g_mutex_lock (&mutex);
>
> - g_string_printf(str,"{\n");
> + g_string_printf(str, "{\n");
>
> GHashTable *ht = cfs_status.vmlist;
> +
> + int res = 0;
> + GString *path = NULL;
> gpointer tmp = NULL;
> + char **values = calloc(num_props, sizeof(char*));
> + char min = 'z', max = 'a';
> +
> + for (uint8_t i = 0; i < num_props; i++) {
> + if (props[i][0] > max) {
> + max = props[i][0];
> + }
> +
> + if (props[i][0] < min) {
> + min = props[i][0];
> + }
> + }
> +
> if (!g_hash_table_size(ht)) {
> goto ret;
> }
> @@ -919,15 +990,8 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
> // use memdb_read_nolock because lock is handled here
> int size = memdb_read_nolock(memdb, path->str, &tmp);
> if (tmp == NULL) goto err;
> - if (size <= prop_len) goto ret;
> -
> - char *val = _get_property_value(tmp, size, prop, prop_len);
> - if (val == NULL) goto ret;
> -
> - g_string_append_printf(str, "\"%u\":{", vmid);
> - _g_str_append_kv_jsonescaped(str, prop, val);
> - g_string_append_c(str, '}');
>
> + _print_found_properties(str, tmp, size, props, num_props, vmid, values, min, max, 1);
> } else {
> GHashTableIter iter;
> g_hash_table_iter_init (&iter, ht);
> @@ -943,21 +1007,16 @@ cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *pro
> tmp = NULL;
> // use memdb_read_nolock because lock is handled here
> int size = memdb_read_nolock(memdb, path->str, &tmp);
> - if (tmp == NULL || size <= prop_len) continue;
> + if (tmp == NULL) continue;
>
> - char *val = _get_property_value(tmp, size, prop, prop_len);
> - if (val == NULL) continue;
> -
> - if (!first) g_string_append_printf(str, ",\n");
> - else first = 0;
> -
> - g_string_append_printf(str, "\"%u\":{", vminfo->vmid);
> - _g_str_append_kv_jsonescaped(str, prop, val);
> - g_string_append_c(str, '}');
> + memset(values, 0, sizeof(char*)*num_props); // reset array
> + first = _print_found_properties(str, tmp, size, props, num_props,
> + vminfo->vmid, values, min, max, first);
> }
> }
> ret:
> g_free(tmp);
> + free(values);
> if (path != NULL) {
> g_string_free(path, TRUE);
> }
> @@ -973,6 +1032,12 @@ enoent:
> goto ret;
> }
>
> +int
> +cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *prop, uint32_t vmid)
> +{
> + return cfs_create_guest_conf_properties_msg(str, memdb, &prop, 1, vmid);
> +}
> +
> void
> record_memdb_change(const char *path)
> {
> diff --git a/data/src/status.h b/data/src/status.h
> index bbf0948..041cb34 100644
> --- a/data/src/status.h
> +++ b/data/src/status.h
> @@ -163,4 +163,7 @@ cfs_create_memberlist_msg(
> int
> cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const char *prop, uint32_t vmid);
>
> +int
> +cfs_create_guest_conf_properties_msg(GString *str, memdb_t *memdb, const char **props, uint8_t num_props, uint32_t vmid);
> +
> #endif /* _PVE_STATUS_H_ */
> --
> 2.30.2
More information about the pve-devel
mailing list