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

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?