* [pve-devel] [PATCH cluster 1/3] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method
@ 2022-03-14 9:03 Dominik Csapak
2022-03-14 9:03 ` [pve-devel] [PATCH cluster 2/3] Cluster: add get_guest_config_properties Dominik Csapak
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Dominik Csapak @ 2022-03-14 9:03 UTC (permalink / raw)
To: pve-devel
for getting multiple properties from the in memory config of the
guests. I added a new CSF_IPC_ call to maintain backwards compatibility.
It basically behaves the same as
CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties
instead.
The old way of getting a single property is now also done by
the new function.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
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)
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(-)
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..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;
+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;
+
typedef struct {
struct qb_ipc_request_header req_header;
char token[];
@@ -348,6 +355,46 @@ 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;
+
+ int propslen = 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 (propslen <= 0) {
+ cfs_debug("propslen <= 0, %d", propslen);
+ result = -EINVAL;
+ } else {
+ const char **properties = malloc(sizeof(char*) * rh->num_props);
+ char *current = (rh->props);
+ int remaining = propslen;
+ for (uint8_t i = 0; i < rh->num_props; i++) {
+ uint8_t proplen = (uint8_t)(*current);
+ if (proplen > --remaining) {
+ cfs_debug("property len > remaining: %d > %d", proplen, remaining);
+ result = -EINVAL;
+ break;
+ }
+ properties[i] = ++current;
+ current[proplen - 1] = '\0'; // ensure property is 0 terminated
+ remaining -= proplen;
+ current += proplen;
+ }
+
+ if (result == 0) {
+ cfs_debug("cfs_get_guest_config_properties: basic valid checked, do request");
+ 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..c73ad11 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
+// 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)
+{
+ 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++;
+
+ 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;
+ }
+
+ 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 format
// restricion 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)
{
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,33 @@ _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) {
+ // found all
+ return;
}
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;
}
*next_newline = '\0';
}
- return NULL; // not found
+ return;
}
static void
@@ -883,24 +902,36 @@ _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)
+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 +950,25 @@ 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, '}');
+ _get_property_values(values, tmp, size, props, num_props, min, max);
+
+ uint8_t found = 0;
+ for (uint8_t i = 0; i < num_props; i++) {
+ if (values[i] != NULL) {
+ if (found) {
+ g_string_append_c(str, ',');
+ } else {
+ 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, '}');
+ }
} else {
GHashTableIter iter;
g_hash_table_iter_init (&iter, ht);
@@ -943,21 +984,34 @@ 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;
-
- 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;
+ if (tmp == 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 = 0;
+ for (uint8_t i = 0; i < num_props; i++) {
+ if (values[i] != NULL) {
+ 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\":{", vminfo->vmid);
+ found = 1;
+ }
+ _g_str_append_kv_jsonescaped(str, props[i], values[i]);
+ }
+ }
- 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 != NULL) {
g_string_free(path, TRUE);
}
@@ -973,6 +1027,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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [pve-devel] [PATCH cluster 2/3] Cluster: add get_guest_config_properties
2022-03-14 9:03 [pve-devel] [PATCH cluster 1/3] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method Dominik Csapak
@ 2022-03-14 9:03 ` Dominik Csapak
2022-03-16 10:08 ` Fabian Grünbichler
2022-03-14 9:03 ` [pve-devel] [PATCH cluster 3/3] Cluster: fix typo Dominik Csapak
2022-03-16 9:52 ` [pve-devel] [PATCH cluster 1/3] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method Fabian Grünbichler
2 siblings, 1 reply; 6+ messages in thread
From: Dominik Csapak @ 2022-03-14 9:03 UTC (permalink / raw)
To: pve-devel
akin to get_guest_config_property, but with a list of properties.
uses the new CFS_IPC_GET_GUEST_CONFIG_PROPERTIES
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
data/PVE/Cluster.pm | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
index 765fb43..c65ba17 100644
--- a/data/PVE/Cluster.pm
+++ b/data/PVE/Cluster.pm
@@ -340,6 +340,27 @@ sub get_node_kv {
return $res;
}
+# properties: an array-ref of config properties you want to get, e.g., this
+# is perfect to get multiple properties of a guest _fast_
+# (>100 faster than manual parsing here)
+# vmid: optional, if a valid is passed we only check that one, else return all
+# NOTE: does *not* searches snapshot and PENDING entries sections!
+sub get_guest_config_properties {
+ my ($properties, $vmid) = @_;
+
+ die "properties required" if !defined($properties);
+
+ my $bindata = pack "VC", $vmid // 0, scalar(@$properties);
+ for my $property (@$properties) {
+ die "property name cannot be longer than 254 chars\n"
+ if length($property) > 254;
+ $bindata .= pack "C/Z*", $property;
+ }
+ my $res = $ipcc_send_rec_json->(CFS_IPC_GET_GUEST_CONFIG_PROPERTIES, $bindata);
+
+ return $res;
+}
+
# property: a config property you want to get, e.g., this is perfect to get
# the 'lock' entry of a guest _fast_ (>100 faster than manual parsing here)
# vmid: optipnal, if a valid is passed we only check that one, else return all
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [pve-devel] [PATCH cluster 3/3] Cluster: fix typo
2022-03-14 9:03 [pve-devel] [PATCH cluster 1/3] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method Dominik Csapak
2022-03-14 9:03 ` [pve-devel] [PATCH cluster 2/3] Cluster: add get_guest_config_properties Dominik Csapak
@ 2022-03-14 9:03 ` Dominik Csapak
2022-03-16 9:52 ` [pve-devel] applied: " Fabian Grünbichler
2022-03-16 9:52 ` [pve-devel] [PATCH cluster 1/3] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method Fabian Grünbichler
2 siblings, 1 reply; 6+ messages in thread
From: Dominik Csapak @ 2022-03-14 9:03 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
data/PVE/Cluster.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
index c65ba17..49022c3 100644
--- a/data/PVE/Cluster.pm
+++ b/data/PVE/Cluster.pm
@@ -363,7 +363,7 @@ sub get_guest_config_properties {
# property: a config property you want to get, e.g., this is perfect to get
# the 'lock' entry of a guest _fast_ (>100 faster than manual parsing here)
-# vmid: optipnal, if a valid is passed we only check that one, else return all
+# vmid: optional, if a valid is passed we only check that one, else return all
# NOTE: does *not* searches snapshot and PENDING entries sections!
sub get_guest_config_property {
my ($property, $vmid) = @_;
--
2.30.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [pve-devel] applied: [PATCH cluster 3/3] Cluster: fix typo
2022-03-14 9:03 ` [pve-devel] [PATCH cluster 3/3] Cluster: fix typo Dominik Csapak
@ 2022-03-16 9:52 ` Fabian Grünbichler
0 siblings, 0 replies; 6+ messages in thread
From: Fabian Grünbichler @ 2022-03-16 9:52 UTC (permalink / raw)
To: Proxmox VE development discussion
On March 14, 2022 10:03 am, Dominik Csapak wrote:
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> data/PVE/Cluster.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
> index c65ba17..49022c3 100644
> --- a/data/PVE/Cluster.pm
> +++ b/data/PVE/Cluster.pm
> @@ -363,7 +363,7 @@ sub get_guest_config_properties {
>
> # property: a config property you want to get, e.g., this is perfect to get
> # the 'lock' entry of a guest _fast_ (>100 faster than manual parsing here)
> -# vmid: optipnal, if a valid is passed we only check that one, else return all
> +# vmid: optional, if a valid is passed we only check that one, else return all
> # NOTE: does *not* searches snapshot and PENDING entries sections!
> sub get_guest_config_property {
> my ($property, $vmid) = @_;
> --
> 2.30.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH cluster 1/3] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method
2022-03-14 9:03 [pve-devel] [PATCH cluster 1/3] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method Dominik Csapak
2022-03-14 9:03 ` [pve-devel] [PATCH cluster 2/3] Cluster: add get_guest_config_properties Dominik Csapak
2022-03-14 9:03 ` [pve-devel] [PATCH cluster 3/3] Cluster: fix typo Dominik Csapak
@ 2022-03-16 9:52 ` Fabian Grünbichler
2 siblings, 0 replies; 6+ messages in thread
From: Fabian Grünbichler @ 2022-03-16 9:52 UTC (permalink / raw)
To: Proxmox VE development discussion
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.
>
> It basically behaves the same as
> CFS_IPC_GET_GUEST_CONFIG_PROPERTY, but takes a list of properties
> instead.
>
> The old way of getting a single property is now also done by
> the new function.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> 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)
>
> 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(-)
>
> 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..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;
>
> +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
and it would make it easier to tell whether something is expected
behaviour or a bug down the line ;) e.g., that the props array is
expected to be a length-prefixed array of property names is not obvious
from this struct. it might also be nicer to pass it as array of property
lengths and array of property names? or alternatively, since we have a
request_size, we could skip the lengths altogether and just check that
we get num_props null-terminated strings within the bounds of the
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(
>
> 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;
> +
> + int propslen = 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 (propslen <= 0) {
> + cfs_debug("propslen <= 0, %d", propslen);
should be <= 1? I mean, a propslen of 1 would mean that remaining down
below starts of as 1 and is immediately decremented to 0, and since
proplen should be checked for 0 there it would be caught as well down
there..
> + result = -EINVAL;
> + } else {
> + const char **properties = malloc(sizeof(char*) * rh->num_props);
> + char *current = (rh->props);
> + int remaining = propslen;
> + for (uint8_t i = 0; i < rh->num_props; i++) {
> + uint8_t proplen = (uint8_t)(*current);
> + if (proplen > --remaining) {
> + cfs_debug("property len > remaining: %d > %d", proplen, remaining);
> + result = -EINVAL;
> + break;
> + }
this needs a check for proplen being 0
> + properties[i] = ++current;
> + current[proplen - 1] = '\0'; // ensure property is 0 terminated
else this does bad stuff (granted, num_props is uint8_t for now so the
scope of damage is limited)
> + remaining -= proplen;
> + current += proplen;
> + }
probably should assert that remaining is 0 now?
> +
> + if (result == 0) {
> + cfs_debug("cfs_get_guest_config_properties: basic valid checked, do request");
> + 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..c73ad11 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
> +// 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)
> +{
> + 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++;
> +
> + 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;
> + }
> +
> + 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 format
(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 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)
> {
> 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,33 @@ _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;
so this old variant here
> + 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;
and this new variant here are pretty different (see comments below)
> 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) {
> + // found all
> + return;
> }
> 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;
> }
> *next_newline = '\0';
> }
>
> - return NULL; // not found
> + return;
> }
>
> static void
> @@ -883,24 +902,36 @@ _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)
> +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';
why 'A' if the RE-comment above and old code says '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];
> + }
> + }
the comment referencing that we base our parsing on the basic key:value
regex from the perl config parser now no longer holds - this is way more
accepting ;)
it's not that tragic for the performance optimization aspect I guess,
but it means that this can read and return a lot more stuff than
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, 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, '}');
the code starting here
> + _get_property_values(values, tmp, size, props, num_props, min, max);
> +
> + uint8_t found = 0;
> + for (uint8_t i = 0; i < num_props; i++) {
> + if (values[i] != NULL) {
> + if (found) {
> + g_string_append_c(str, ',');
> + } else {
> + 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, '}');
> + }
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, 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;
> -
> - 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;
> + if (tmp == 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 = 0;
> + for (uint8_t i = 0; i < num_props; i++) {
> + if (values[i] != NULL) {
> + if (found) {
> + g_string_append_c(str, ',');
> + } else {
> + if (!first) g_string_append_printf(str, ",\n");
> + else first = 0;
these two lines for concatenating results in the multiple-guest case is
extra. might be worthy to make a _print_property_values that has a
*first parameter - the single vmid case can then just set that to &0
from the start..
> + g_string_append_printf(str, "\"%u\":{", vminfo->vmid);
and this line takes the vmid from vminfo, but that can trivially be
handled as well since it would be a parameter of the print helper..
> + found = 1;
> + }
> + _g_str_append_kv_jsonescaped(str, props[i], values[i]);
> + }
> + }
>
> - 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 != NULL) {
> g_string_free(path, TRUE);
> }
> @@ -973,6 +1027,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
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH cluster 2/3] Cluster: add get_guest_config_properties
2022-03-14 9:03 ` [pve-devel] [PATCH cluster 2/3] Cluster: add get_guest_config_properties Dominik Csapak
@ 2022-03-16 10:08 ` Fabian Grünbichler
0 siblings, 0 replies; 6+ messages in thread
From: Fabian Grünbichler @ 2022-03-16 10:08 UTC (permalink / raw)
To: Proxmox VE development discussion
this and the other one existing one might also warrant a comment
indicating that you possibly still want to parse/validate/filter the
result before passing it along further up the stack (or that it should
only be used for very simple keys?)
- if no vmid is passed, the result needs to be filtered by access to not
leak information
- if property is anything but a very simple type, validation might be
important (for lock it's not that bad since that is just a simple
enum, so a bogus/invalid value will likely just look weird, for the
`tags` property it's also not that bad since those are just
simple lists of strings, but who knows what this will get used for
down the line ;))
also depending on whether the msg format gets changed this will need
adapation obviously.
On March 14, 2022 10:03 am, Dominik Csapak wrote:
> akin to get_guest_config_property, but with a list of properties.
> uses the new CFS_IPC_GET_GUEST_CONFIG_PROPERTIES
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> data/PVE/Cluster.pm | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
> index 765fb43..c65ba17 100644
> --- a/data/PVE/Cluster.pm
> +++ b/data/PVE/Cluster.pm
> @@ -340,6 +340,27 @@ sub get_node_kv {
> return $res;
> }
>
> +# properties: an array-ref of config properties you want to get, e.g., this
> +# is perfect to get multiple properties of a guest _fast_
> +# (>100 faster than manual parsing here)
> +# vmid: optional, if a valid is passed we only check that one, else return all
> +# NOTE: does *not* searches snapshot and PENDING entries sections!
> +sub get_guest_config_properties {
> + my ($properties, $vmid) = @_;
> +
> + die "properties required" if !defined($properties);
> +
> + my $bindata = pack "VC", $vmid // 0, scalar(@$properties);
length of $properties actually has a limit which should maybe be checked
here as well? I mean it is rather unlikely to be misused in practice,
and will print a warning that the `C` wraps the value here, but..
> + for my $property (@$properties) {
> + die "property name cannot be longer than 254 chars\n"
> + if length($property) > 254;
> + $bindata .= pack "C/Z*", $property;
like indicated in the other patch's comments, this is a rather strange
encoding (flashbacks to ASN.1 ;)) and shouldn't be needed.
> + }
> + my $res = $ipcc_send_rec_json->(CFS_IPC_GET_GUEST_CONFIG_PROPERTIES, $bindata);
> +
> + return $res;
> +}
> +
> # property: a config property you want to get, e.g., this is perfect to get
> # the 'lock' entry of a guest _fast_ (>100 faster than manual parsing here)
> # vmid: optipnal, if a valid is passed we only check that one, else return all
> --
> 2.30.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-16 10:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-14 9:03 [pve-devel] [PATCH cluster 1/3] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method Dominik Csapak
2022-03-14 9:03 ` [pve-devel] [PATCH cluster 2/3] Cluster: add get_guest_config_properties Dominik Csapak
2022-03-16 10:08 ` Fabian Grünbichler
2022-03-14 9:03 ` [pve-devel] [PATCH cluster 3/3] Cluster: fix typo Dominik Csapak
2022-03-16 9:52 ` [pve-devel] applied: " Fabian Grünbichler
2022-03-16 9:52 ` [pve-devel] [PATCH cluster 1/3] add CFS_IPC_GET_GUEST_CONFIG_PROPERTIES method Fabian Grünbichler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox