[Libvirt-cim] [PATCH 2/5] libxkutil: Support for device addresses

[John Ferlan]

On 11/13/2013 11:24 AM, Viktor Mihajlovski wrote:
> On 11/12/2013 11:34 PM, John Ferlan 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
> 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