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

[Wolfgang Bumiller]

On Wed, Jun 12, 2019 at 01:48:57PM +0200, Thomas Lamprecht wrote:
> On 6/12/19 1:12 PM, Wolfgang Bumiller wrote:
> > On Tue, Jun 11, 2019 at 06:02:22AM +0200, Thomas Lamprecht wrote:
> >> +
> >> +		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..

Sounds good to me. In the backend there shouldn't be double quotes after
all, just the documented regex you posted in the comment/commit message
doesn't reflect that as it just matches (.+) for the value.

> (...)
> >> +
> >> +			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.

This issue exists in many projects ;-)
We've been trying to improve that situation in lxc as well as we go
along with other patches - if we notice it ;-)