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 204E86A3A8 for ; Wed, 16 Mar 2022 10:53:26 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 175F925CAB for ; Wed, 16 Mar 2022 10:52:56 +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 id 3FBEC25CA2 for ; Wed, 16 Mar 2022 10:52:54 +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 1907446C62 for ; Wed, 16 Mar 2022 10:52:54 +0100 (CET) Date: Wed, 16 Mar 2022 10:52:43 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20220314090307.1130083-1-d.csapak@proxmox.com> In-Reply-To: <20220314090307.1130083-1-d.csapak@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1647419350.yd1qp5xipr.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.185 Adjusted score from AWL reputation of From: address 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 Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH cluster 1/3] 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: Wed, 16 Mar 2022 09:53:26 -0000 On March 14, 2022 10:03 am, Dominik Csapak wrote: > for getting multiple properties from the in memory config of the > guests. I added a new CSF_IPC_ call to maintain backwards compatibility. >=20 > It basically behaves the same as > CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties > instead. >=20 > The old way of getting a single property is now also done by > the new function. >=20 > Signed-off-by: Dominik Csapak > --- > in my benchmarks with ~10000 "real" vm configs, i could not really > see a difference for the old code vs the new with 2 properties > (~30ms per call) >=20 > data/src/cfs-ipc-ops.h | 2 + > data/src/server.c | 47 ++++++++++++ > data/src/status.c | 166 ++++++++++++++++++++++++++++------------- > data/src/status.h | 3 + > 4 files changed, 165 insertions(+), 53 deletions(-) >=20 > 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 @@ > =20 > #define CFS_IPC_VERIFY_TOKEN 12 > =20 > +#define CFS_IPC_GET_GUEST_CONFIG_PROPERTIES 13 > + > #endif > diff --git a/data/src/server.c b/data/src/server.c > index 549788a..d4f6374 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; > =20 > +typedef struct { > + struct qb_ipc_request_header req_header; > + uint32_t vmid; > + uint8_t num_props; > + char props[]; > +} cfs_guest_config_properties_get_request_header_t; a description here of what this is expected to contain would be nice. it's derivable from the code below, but it's not trivial to read IMHO=20 and it would make it easier to tell whether something is expected=20 behaviour or a bug down the line ;) e.g., that the props array is=20 expected to be a length-prefixed array of property names is not obvious=20 from this struct. it might also be nicer to pass it as array of property=20 lengths and array of property names? or alternatively, since we have a=20 request_size, we could skip the lengths altogether and just check that=20 we get num_props null-terminated strings within the bounds of the=20 request (via a loop and strnlen)? > + > typedef struct { > struct qb_ipc_request_header req_header; > char token[]; > @@ -348,6 +355,46 @@ static int32_t s1_msg_process_fn( > =20 > result =3D cfs_create_guest_conf_property_msg(outbuf, memdb, rh->prop= erty, rh->vmid); > } > + } else if (request_id =3D=3D CFS_IPC_GET_GUEST_CONFIG_PROPERTIES) { > + > + cfs_guest_config_properties_get_request_header_t *rh =3D > + (cfs_guest_config_properties_get_request_header_t *) data; > + > + int propslen =3D request_size - G_STRUCT_OFFSET(cfs_guest_config_prope= rties_get_request_header_t, props); > + > + result =3D 0; > + if (rh->vmid < 100 && rh->vmid !=3D 0) { > + cfs_debug("vmid out of range %u", rh->vmid); > + result =3D -EINVAL; > + } else if (rh->vmid >=3D 100 && !vmlist_vm_exists(rh->vmid)) { > + result =3D -ENOENT; > + } else if (propslen <=3D 0) { > + cfs_debug("propslen <=3D 0, %d", propslen); should be <=3D 1? I mean, a propslen of 1 would mean that remaining down=20 below starts of as 1 and is immediately decremented to 0, and since=20 proplen should be checked for 0 there it would be caught as well down=20 there.. > + result =3D -EINVAL; > + } else { > + const char **properties =3D malloc(sizeof(char*) * rh->num_props); > + char *current =3D (rh->props); > + int remaining =3D propslen; > + for (uint8_t i =3D 0; i < rh->num_props; i++) { > + uint8_t proplen =3D (uint8_t)(*current); > + if (proplen > --remaining) { > + cfs_debug("property len > remaining: %d > %d", proplen, remaining); > + result =3D -EINVAL; > + break; > + } this needs a check for proplen being 0 > + properties[i] =3D ++current; > + current[proplen - 1] =3D '\0'; // ensure property is 0 terminated else this does bad stuff (granted, num_props is uint8_t for now so the=20 scope of damage is limited) > + remaining -=3D proplen; > + current +=3D proplen; > + } probably should assert that remaining is 0 now? > + > + if (result =3D=3D 0) { > + cfs_debug("cfs_get_guest_config_properties: basic valid checked, = do request"); > + result =3D cfs_create_guest_conf_properties_msg(outbuf, memdb, pr= operties, rh->num_props, rh->vmid); > + } > + > + free(properties); > + } > } else if (request_id =3D=3D CFS_IPC_VERIFY_TOKEN) { > =20 > cfs_verify_token_request_header_t *rh =3D (cfs_verify_token_request_he= ader_t *) data; > diff --git a/data/src/status.c b/data/src/status.c > index 9bceaeb..c73ad11 100644 > --- a/data/src/status.c > +++ b/data/src/status.c > @@ -804,25 +804,52 @@ cfs_create_vmlist_msg(GString *str) > return 0; > } > =20 > -// checks the conf for a line starting with '$prop:' and returns the val= ue > +// 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 star= ts > +char * > +_get_property_value_from_line(char *line, int line_len, const char *prop= , int prop_len) > +{ > + if (line_len <=3D prop_len + 1) return NULL; > + > + if (line[prop_len] =3D=3D ':' && memcmp(line, prop, prop_len) =3D=3D 0)= { // found > + char *v_start =3D &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 =3D &line[line_len - 1]; > + while (v_end > v_start && isspace(*v_end)) v_end--; > + v_end[1] =3D '\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, whitout initial whitespace(s), we only deal with the form= at (pre-existing) typo 'without' (without? white out? ;)) > // restricion imposed by our perl VM config parser, main reference is (pre-existing) typo 'restricion' > // PVE::QemuServer::parse_vm_config this allows to be very fast and stil= l > // relatively simple > -// main restrictions used for our advantage is the properties match rege= s: > +// main restrictions used for our advantage is the properties match rege= x: > // ($line =3D~ 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 pro= p_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) > { > const char *const conf_end =3D conf + conf_size; > char *line =3D conf; > - size_t remaining_size; > + size_t remaining_size =3D conf_size; > + int count =3D 0; > =20 > char *next_newline =3D memchr(conf, '\n', conf_size); > if (next_newline =3D=3D NULL) { > - return NULL; // valid property lines end with \n, but none in the conf= ig > + return; // valid property lines end with \n, but none in the config > } > *next_newline =3D '\0'; > =20 > @@ -830,41 +857,33 @@ _get_property_value(char *conf, int conf_size, cons= t char *prop, int prop_len) > if (!line[0]) goto next; > =20 > // snapshot or pending section start, but nothing found yet -> not fou= nd > - if (line[0] =3D=3D '[') return NULL; > - // properties start with /^[a-z]/, so continue early if not > - if (line[0] < 'a' || line[0] > 'z') goto next; so this old variant here > + if (line[0] =3D=3D '[') 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; and this new variant here are pretty different (see comments below) > int line_len =3D strlen(line); > - if (line_len <=3D prop_len + 1) goto next; > - > - if (line[prop_len] =3D=3D ':' && memcmp(line, prop, prop_len) =3D=3D 0= ) { // found > - char *v_start =3D &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 =3D &line[line_len - 1]; > - while (v_end > v_start && isspace(*v_end)) v_end--; > - v_end[1] =3D '\0'; > - > - return v_start; > + for (uint8_t i =3D 0; i < num_props; i++) { > + char * value =3D _get_property_value_from_line(line, line_len, props[= i], strlen(props[i])); > + if (value !=3D NULL) { > + count +=3D (found[i] !=3D NULL) & 0x1; // count newly found lines > + found[i] =3D value; > + } > + } > + if (count =3D=3D num_props) { > + // found all > + return; > } > next: > line =3D next_newline + 1; > remaining_size =3D conf_end - line; > - if (remaining_size <=3D prop_len) { > - return NULL; > - } > next_newline =3D memchr(line, '\n', remaining_size); > if (next_newline =3D=3D NULL) { > - return NULL; // valid property lines end with \n, but none in the con= fig > + return; > } > *next_newline =3D '\0'; > } > =20 > - return NULL; // not found > + return; > } > =20 > static void > @@ -883,24 +902,36 @@ _g_str_append_kv_jsonescaped(GString *str, const ch= ar *k, const char *v) > } > =20 > int > -cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const c= har *prop, uint32_t vmid) > +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 !=3D NULL, -EINVAL); > g_return_val_if_fail(str !=3D NULL, -EINVAL); > =20 > - int prop_len =3D strlen(prop); > - int res =3D 0; > - GString *path =3D NULL; > - > // Prelock &memdb->mutex in order to enable the usage of memdb_read_nol= ock > // to prevent Deadlocks as in #2553 > g_mutex_lock (&memdb->mutex); > g_mutex_lock (&mutex); > =20 > - g_string_printf(str,"{\n"); > + g_string_printf(str, "{\n"); > =20 > GHashTable *ht =3D cfs_status.vmlist; > + > + int res =3D 0; > + GString *path =3D NULL; > gpointer tmp =3D NULL; > + char **values =3D calloc(num_props, sizeof(char*)); > + char min =3D 'z', max =3D 'A'; why 'A' if the RE-comment above and old code says 'a'? > + > + for (uint8_t i =3D 0; i < num_props; i++) { > + if (props[i][0] > max) { > + max =3D props[i][0]; > + } > + > + if (props[i][0] < min) { > + min =3D props[i][0]; > + } > + } the comment referencing that we base our parsing on the basic key:value=20 regex from the perl config parser now no longer holds - this is way more=20 accepting ;) it's not that tragic for the performance optimization aspect I guess,=20 but it means that this can read and return a lot more stuff than=20 expected? we could at least clamp it to a-z, that's not too expensive.. > + > if (!g_hash_table_size(ht)) { > goto ret; > } > @@ -919,15 +950,25 @@ cfs_create_guest_conf_property_msg(GString *str, me= mdb_t *memdb, const char *pro > // use memdb_read_nolock because lock is handled here > int size =3D memdb_read_nolock(memdb, path->str, &tmp); > if (tmp =3D=3D NULL) goto err; > - if (size <=3D prop_len) goto ret; > =20 > - char *val =3D _get_property_value(tmp, size, prop, prop_len); > - if (val =3D=3D NULL) goto ret; > - > - g_string_append_printf(str, "\"%u\":{", vmid); > - _g_str_append_kv_jsonescaped(str, prop, val); > - g_string_append_c(str, '}'); the code starting here > + _get_property_values(values, tmp, size, props, num_props, min, max); > + > + uint8_t found =3D 0; > + for (uint8_t i =3D 0; i < num_props; i++) { > + if (values[i] !=3D NULL) { > + if (found) { > + g_string_append_c(str, ','); > + } else { > + g_string_append_printf(str, "\"%u\":{", vmid); > + found =3D 1; > + } > + _g_str_append_kv_jsonescaped(str, props[i], values[i]); > + } > + } > =20 > + if (found) { > + g_string_append_c(str, '}'); > + } until here is pretty much duplicated below, with just one exception > } else { > GHashTableIter iter; > g_hash_table_iter_init (&iter, ht); > @@ -943,21 +984,34 @@ cfs_create_guest_conf_property_msg(GString *str, me= mdb_t *memdb, const char *pro > tmp =3D NULL; > // use memdb_read_nolock because lock is handled here > int size =3D memdb_read_nolock(memdb, path->str, &tmp); > - if (tmp =3D=3D NULL || size <=3D prop_len) continue; > - > - char *val =3D _get_property_value(tmp, size, prop, prop_len); > - if (val =3D=3D NULL) continue; > - > - if (!first) g_string_append_printf(str, ",\n"); > - else first =3D 0; > + if (tmp =3D=3D NULL) continue; > + > + memset(values, 0, sizeof(char*)*num_props); // reset array > + _get_property_values(values, tmp, size, props, num_props, min, max); > + > + uint8_t found =3D 0; > + for (uint8_t i =3D 0; i < num_props; i++) { > + if (values[i] !=3D NULL) { > + if (found) { > + g_string_append_c(str, ','); > + } else { > + if (!first) g_string_append_printf(str, ",\n"); > + else first =3D 0; these two lines for concatenating results in the multiple-guest case is=20 extra. might be worthy to make a _print_property_values that has a=20 *first parameter - the single vmid case can then just set that to &0=20 from the start.. > + g_string_append_printf(str, "\"%u\":{", vminfo->vmid); and this line takes the vmid from vminfo, but that can trivially be=20 handled as well since it would be a parameter of the print helper.. > + found =3D 1; > + } > + _g_str_append_kv_jsonescaped(str, props[i], values[i]); > + } > + } > =20 > - g_string_append_printf(str, "\"%u\":{", vminfo->vmid); > - _g_str_append_kv_jsonescaped(str, prop, val); > - g_string_append_c(str, '}'); > + if (found) { > + g_string_append_c(str, '}'); > + } > } > } > ret: > g_free(tmp); > + free(values); > if (path !=3D NULL) { > g_string_free(path, TRUE); > } > @@ -973,6 +1027,12 @@ enoent: > goto ret; > } > =20 > +int > +cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, const c= har *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 c= har *prop, uint32_t vmid); > =20 > +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_ */ > --=20 > 2.30.2 >=20 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 >=20 >=20