From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id DCB549191A for ; Mon, 14 Nov 2022 14:16:15 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B739923A59 for ; Mon, 14 Nov 2022 14:15:45 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Mon, 14 Nov 2022 14:15:43 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 4218343972 for ; Mon, 14 Nov 2022 14:15:43 +0100 (CET) Date: Mon, 14 Nov 2022 14:15:40 +0100 From: Wolfgang Bumiller To: Dominik Csapak Cc: pve-devel@lists.proxmox.com Message-ID: <20221114131540.a3p4hcavgyopvxp4@wobu-vie.proxmox.com> References: <20221114094404.1241050-1-d.csapak@proxmox.com> <20221114094404.1241050-2-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221114094404.1241050-2-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: =?UTF-8?Q?0=0A=09?=AWL 0.237 Adjusted score from AWL reputation of From: =?UTF-8?Q?address=0A=09?=BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict =?UTF-8?Q?Alignment=0A=09?=SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF =?UTF-8?Q?Record=0A=09?=SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH cluster v9 1/4] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Nov 2022 13:16:15 -0000 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 > --- > 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