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
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
