[pve-devel] [PATCH cluster] add get_guest_config_property IPCC method

[Thomas Lamprecht]

On 6/12/19 1:12 PM, Wolfgang Bumiller wrote:
> On Tue, Jun 11, 2019 at 06:02:22AM +0200, Thomas Lamprecht wrote:
>> [snip]
>>  
>> +// 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
>> +// 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:
>> +// ($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, char *prop, int prop_len)
> 
> prop should be const
> 

ok

>> +{
>> +	char *line, *temp;
> 
> initializing `temp` to NULL shutsup a warning gcc seems to spit out on
> my end here...
> status.c:816:10: error: ‘__s’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>     char *v_start = &line[prop_len + 1];
> 
> `__s` seems to come from inlining strtok or some such, a gcc bug
> apparently...

ah yes, I remember that, I'm currently on Buster with gcc-8, which
has this behaviour fixed, but I can set it to NULL just fine.

> 
> 
>> +
>> +	line = strtok_r(conf, "\n", &temp);
>> +	while (line != NULL) {
>> +		if (!line[0]) goto next;
>> +
>> +		// snapshot or pending section start and nothing found yet
>> +		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] == ':' && strncmp(line, prop, prop_len) == 0) { // found
> 
> This strncmp could be a memcmp given all the checks above (particularly
> the strlen() check). strncmp() checks for null bytes, memcmp doesn't
> care (and we already verified with strlen() that there aren't any null
> bytes within [0..prop_len].

OK

> 
>> +			char *v_start = &line[prop_len + 1];
>> +
>> +			// drop initial value whitespaces here already
>> +			while (*v_start && isspace(*v_start)) v_start++;
>> +
>> +			// TODO: drop trailing whitespace here too??
> 
> We already have `line_len`, so going backwards from that should be
> efficient enough.

yes, was just to lazy at the end, will do

> 
>> +
>> +			if (!*v_start) return NULL;
>> +			return v_start;
>> +		}
>> +next:
>> +		line = strtok_r(NULL, "\n", &temp);
>> +	}
>> +
>> +	return NULL; // not found
>> +}
>> +
>> +int
>> +cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, char *prop, uint32_t vmid)
> 
> prop should be const
> 

OK

>> +{
>> +	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;
>> +
>> +	g_mutex_lock (&mutex);
>> +
>> +	g_string_printf(str,"{\n");
>> +
>> +	GHashTable *ht = cfs_status.vmlist;
>> +	if (!g_hash_table_size(ht)) {
>> +		goto ret;
>> +	}
>> +
>> +	path = g_string_new_len(NULL, 256);
>> +	if (vmid >= 100) {
>> +		vminfo_t *vminfo = (vminfo_t *) g_hash_table_lookup(cfs_status.vmlist, &vmid);
>> +		if (vminfo == NULL) goto enoent;
>> +
>> +		if (!vminfo_to_path(vminfo, path)) goto err;
>> +
>> +		gpointer tmp = NULL;
>> +		int size = memdb_read(memdb, path->str, &tmp);
>> +		if (tmp == NULL || size <= prop_len) goto err;
> 
> The 'or' part of the condition would have a valid `tmp` pointer needing
> to be `g_free()`d, no? So maybe just unconditionally call free before
> the goto?

yes, true that, thanks for catching

> 
>> +
>> +		char *val = _get_property_value(tmp, prop, prop_len);
>> +		if (val == NULL) {
>> +			g_free(tmp);
>> +			goto ret;
>> +		}
>> +
>> +		g_string_append_printf(str,"\"%u\": { \"%s\": \"%s\"\n }", vmid, prop, val);
> 
> Should we not sanity-check the value for double quotes here?

we normally do not have any here, but we can have in theory..

Maybe do the warn-and-ignore approach for now? and if we really
need it directly go to a libjson approach..

> 
>> +		g_free(tmp);
>> +
>> +	} else {
>> +		GHashTableIter iter;
>> +		g_hash_table_iter_init (&iter, ht);
>> +
>> +		gpointer key, value;
>> +		int first = 1;
>> +		while (g_hash_table_iter_next (&iter, &key, &value)) {
>> +			vminfo_t *vminfo = (vminfo_t *)value;
>> +
>> +			if (!vminfo_to_path(vminfo, path)) goto err;
>> +
>> +			gpointer tmp = NULL;
>> +			int size = memdb_read(memdb, path->str, &tmp);
>> +			if (tmp == NULL || size <= prop_len) continue;
> 
> Same g_free(tmp) as above

OK.

> 
>> +
>> +			char *val = _get_property_value(tmp, prop, prop_len);
>> +			if (val == NULL) {
>> +				g_free(tmp);
>> +				continue;
>> +			}
>> +
>> +			if (!first) g_string_append_printf(str, ",\n");
>> +			else first = 0;
>> +
>> +			g_string_append_printf(str,"\"%u\": {\"%s\": \"%s\"}", vminfo->vmid, prop, val);
>> +			g_free(tmp);
>> +		}
>> +	}
>> +ret:
>> +	g_string_free(path, TRUE);
>> +	g_string_append_printf(str,"\n}\n");
>> +	g_mutex_unlock (&mutex);
>> +
>> +	return res;
>> +err:
>> +	res = -1;
> 
> I'm not a fan of mixing hardcoded values and errno values. This
> corresponds to EPERM which is a rather weird error in those cases.
> Granted, there is often no good error code, so I'd just stick to EINVAL
> (or EIO to distinguish it from the preconditions).

OK, was a bit oriented on other functions we have here, but seems
reasonable to me.

> 
>> +	goto ret;
>> +enoent:
>> +	res = -ENOENT;
>> +	goto ret;
>> +}
>> +
>>  void
>>  record_memdb_change(const char *path)
>>  {
>> diff --git a/data/src/status.h b/data/src/status.h
>> index 2e4153a..bda3e36 100644
>> --- a/data/src/status.h
>> +++ b/data/src/status.h
>> @@ -25,6 +25,7 @@
>>  
>>  #include "dfsm.h"
>>  #include "logger.h"
>> +#include "memdb.h"
>>  
>>  #define VMTYPE_QEMU 1
>>  #define VMTYPE_OPENVZ 2
>> @@ -158,5 +159,7 @@ int
>>  cfs_create_memberlist_msg(
>>  	GString *str);
>>  
>> +int
>> +cfs_create_guest_conf_property_msg(GString *str, memdb_t *memdb, char *prop, uint32_t vmid);
>>  
>>  #endif /* _PVE_STATUS_H_ */
>> -- 
>> 2.20.1