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 67DF78983 for ; Wed, 16 Nov 2022 10:50:41 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4AC051D2EF for ; Wed, 16 Nov 2022 10:50:41 +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 ; Wed, 16 Nov 2022 10:50:39 +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 AF73144B71 for ; Wed, 16 Nov 2022 10:50:39 +0100 (CET) Date: Wed, 16 Nov 2022 10:50:38 +0100 From: Wolfgang Bumiller To: Dominik Csapak Cc: pve-devel@lists.proxmox.com Message-ID: <20221116095038.vrvn43aeq33vrame@casey.proxmox.com> References: <20221115130248.1007325-1-d.csapak@proxmox.com> <20221115130248.1007325-2-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221115130248.1007325-2-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: =?UTF-8?Q?0=0A=09?=AWL 0.234 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 v10 1/5] 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 Nov 2022 09:50:41 -0000 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 > --- Sort-of Acked-by: Wolfgang Bumiller 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