[libvirt] [PATCH RFC 4/7] libxl: implement virDomainBlockStats

[Jim Fehlig]

Joao Martins wrote:
> On 09/23/2015 11:24 PM, Jim Fehlig wrote:
>   
>> Joao Martins wrote:
>>     
[...]
>>> +static int
>>> +libxlDomainBlockStatsVBD(virDomainObjPtr vm,
>>> +                         const char *dev,
>>> +                         libxlBlockStatsPtr stats)
>>> +{
>>> +    int ret = -1;
>>> +    int devno = libxlDiskPathParse(dev, NULL, NULL);
>>> +    int size = libxlDiskSectorSize(vm->def->id, devno);
>>> +#ifdef __linux__
>>> +    char *path, *name, *val;
>>> +    unsigned long long stat;
>>> +
>>> +    if (VIR_STRDUP(stats->backend, "vbd") < 0) {
>>> +        virReportError(VIR_ERR_OPERATION_INVALID,
>>> +                       "%s", _("cannot set backend"));
>>>   
>>>       
>> VIR_STRDUP() already reports OOM error, which is about the only one it
>> can encounter.
>>
>>     
> OK. Two other issues that I already have fixed for v2: 1) libxlDiskPathParse is
> renamed to libxlDiskPathToID which is based on virDiskNameParse (as suggested by
> Daniel) resulting in a much simpler function; 2) Added validation devno;
>
>   
>>> +        return -1;
>>> +    }
>>> +
>>> +    ret = virAsprintf(&path, "/sys/bus/xen-backend/devices/vbd-%d-%d/statistics",
>>> +                vm->def->id, devno);
>>>   
>>>       
>> The usual pattern is 'if (virAsprintf(...) < 0)'.
>>
>>     
> OK.
>
>   
>>> +
>>> +    if (!virFileExists(path)) {
>>> +        virReportError(VIR_ERR_OPERATION_INVALID,
>>> +                       "%s", _("cannot open bus path"));
>>>   
>>>       
>> I think the error should be VIR_ERR_OPERATION_FAILED.
>>
>>     
> OK.
>
>   
>>> +        goto cleanup;
>>> +    }
>>> +
>>> +#define LIBXL_SET_VBDSTAT(FIELD, VAR, MUL)            \
>>>   
>>>       
>> Indentation is off here. Fails 'make syntax-check' with cppi installed.
>>
>>     
>>> +    if ((virAsprintf(&name, "%s/"FIELD, path) < 0) || \
>>> +        (virFileReadAll(name, 256, &val) < 0) ||       \
>>> +        (sscanf(val, "%llu", &stat) != 1)) {          \
>>> +        virReportError(VIR_ERR_OPERATION_INVALID, \
>>> +                       _("cannot read %s"), name);    \
>>>   
>>>       
>> VIR_ERR_OPERATION_FAILED?
>>
>>     
> OK.
>
>   
>>> +        goto cleanup;                                 \
>>> +    }                                                 \
>>> +    VAR += (stat * MUL);                              \
>>> +    VIR_FREE(name);                                   \
>>> +    VIR_FREE(val);
>>> +
>>> +    LIBXL_SET_VBDSTAT("f_req",  stats->f_req,  1)
>>> +    LIBXL_SET_VBDSTAT("wr_req", stats->wr_req, 1)
>>> +    LIBXL_SET_VBDSTAT("rd_req", stats->rd_req, 1)
>>> +    LIBXL_SET_VBDSTAT("wr_sect", stats->wr_bytes, size)
>>> +    LIBXL_SET_VBDSTAT("rd_sect", stats->rd_bytes, size)
>>> +
>>> +    LIBXL_SET_VBDSTAT("ds_req", stats->u.vbd.ds_req, size)
>>> +    LIBXL_SET_VBDSTAT("oo_req", stats->u.vbd.oo_req, 1)
>>> +    ret = 0;
>>> +
>>> + cleanup:
>>> +    VIR_FREE(name);
>>> +    VIR_FREE(path);
>>> +    VIR_FREE(val);
>>>   
>>>       
>> This will only cleanup 'name' and 'val' from the last invocation of the
>> LIBXL_SET_VBDSTAT macro. 'name' and 'val' from the prior invocations
>> will be leaked.
>>
>>     
> The macro will always free 'name' and 'val' on successfull (prior?) invocations,
> thus only the last one needs to be taken care of isn't it?
>   

Opps, I missed the freeing in the macro. But the last expansion includes
freeing 'name' and 'val' too, so those would be double freed in cleanup.
However, it is safe to use the VIR_FREE macro in this way, so looks like
your code is correct afterall :-).

Regards,
Jim