On 11/13/2013 11:24 AM, Viktor Mihajlovski wrote:
+int add_device_address_property(struct device_address *devaddr, + const char *key, + const char *value) +{ + char *k = NULL; + char *v = NULL; + char **list = NULL; + + if (key != NULL && value != NULL) { + k = strdup(key); + v = strdup(value); + if (k == NULL || v == NULL) + goto err; + + list = realloc(devaddr->key, sizeof(char*) * (devaddr->ct+1)); + if (list == NULL) + goto err; + devaddr->key = list; + + list = realloc(devaddr->value, sizeof(char*) * (devaddr->ct+1)); + if (list == NULL)
There's a leak here since 'ct' isn't incremented until later, devaddr->key will have the 'list' from above and that'll either be lost or overwritten. I have to admit that you're not the first reviewer I have confused with this approach ... so: If realloc of the 'value' list fails, the devaddr content remains valid and unchanged, exactly because we don't increment
On 11/12/2013 11:34 PM, John Ferlan wrote: [...] the counter. And the intent is to preserve the passed-in devaddr even if adding new key-value pairs fails. It's true that the 'key' list will have an excess element, which doesn't hurt as it doesn't need to be freed (it has not been set). Frankly, I don't have a better idea, unless not to use realloc but do malloc's followed by memcpy's instead.
oh - right... The next time realloc comes along it could essentially realloc something of the same size which I supposed is a noop this is why I like the libvirt macros around memory (re)allocation and list manipulation. I never have to think about them... I'll push this series in a bit... Tks, John