On Thu, Sep 20, 2018 at 05:34:37PM -0400, John Ferlan wrote:
> Commit 40b5c99a modified the virConfGetValue callers to use
> virConfGetValueString. However, using the virConfGetValueString
> resulted in leaking the returned @value string in each case.
> So, let's modify each instance to use the VIR_AUTOFREE(char *)
> syntax. In some instances changing the variable name since
> @value was used more than once.
>
> Found by Coverity
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
...
> static int
> lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
> {
> - char *value = NULL;
> + VIR_AUTOFREE(char *) hard_limit = NULL;
> + VIR_AUTOFREE(char *) soft_limit = NULL;
> + VIR_AUTOFREE(char *) swap_hard_limit = NULL;
> unsigned long long size = 0;
This is just a matter of taste, but I'd probably went with VIR_AUTOFREE(char *)
value within each of the 'if' blocks.
>
> if (virConfGetValueString(properties,
> "lxc.cgroup.memory.limit_in_bytes",
> - &value) > 0) {
> - if (lxcConvertSize(value, &size) < 0)
> - goto error;
> + &hard_limit) > 0) {
> + if (lxcConvertSize(hard_limit, &size) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse integer: '%s'"),
> + hard_limit);
lxcConvertSize already reports an error: "failed to convert size: '%s'". And
since the one you added provides essentially the same amount of information, we
might as well go with that one. Potentially, we could also change it so that it
matches the wording of all the other ones for consistency reasons, i.e. the one
you're proposing here.
Moreover, I don't really like copying the same error message, having a goto
error label is IMHO justified in case like these, although, we can drop it
specifically for this function, it's different for the functions below though...
> + return -1;
> + }
> size = size / 1024;
> virDomainDefSetMemoryTotal(def, size);
> def->mem.hard_limit = virMemoryLimitTruncate(size);
> @@ -771,76 +777,85 @@ lxcSetMemTune(virDomainDefPtr def, virConfPtr properties)
>
> if (virConfGetValueString(properties,
> "lxc.cgroup.memory.soft_limit_in_bytes",
> - &value) > 0) {
> - if (lxcConvertSize(value, &size) < 0)
> - goto error;
> + &soft_limit) > 0) {
> + if (lxcConvertSize(soft_limit, &size) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse integer: '%s'"),
> + soft_limit);
> + return -1;
> + }
> def->mem.soft_limit = virMemoryLimitTruncate(size / 1024);
> }
>
> if (virConfGetValueString(properties,
> "lxc.cgroup.memory.memsw.limit_in_bytes",
> - &value) > 0) {
> - if (lxcConvertSize(value, &size) < 0)
> - goto error;
> + &swap_hard_limit) > 0) {
> + if (lxcConvertSize(swap_hard_limit, &size) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse integer: '%s'"),
> + swap_hard_limit);
> + return -1;
> + }
> def->mem.swap_hard_limit = virMemoryLimitTruncate(size / 1024);
> }
> return 0;
> -
> - error:
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("failed to parse integer: '%s'"), value);
> - return -1;
we can definitely drop it ^here...Alternatively, we could drop the message in
lxcConvertSize and leave the error label here as well, again, for consistency
reasons, it's perhaps also the better way to let it behave like just like the
virStrToX helpers.
> -
> }
>
> static int
> lxcSetCpuTune(virDomainDefPtr def, virConfPtr properties)
> {
> - char *value = NULL;
> + VIR_AUTOFREE(char *) shares = NULL;
> + VIR_AUTOFREE(char *) quota = NULL;
> + VIR_AUTOFREE(char *) period = NULL;
>
> if (virConfGetValueString(properties, "lxc.cgroup.cpu.shares",
> - &value) > 0) {
again, matter of taste, but I think that having a helper VIR_AUTOFREE(char *)
value that gets destroyed after each block looks more clean than having three
different function local helper variables just to match the structure members
we're filling in.
> - if (virStrToLong_ull(value, NULL, 10, &def->cputune.shares) < 0)
> - goto error;
> + &shares) > 0) {
> + if (virStrToLong_ull(shares, NULL, 10, &def->cputune.shares) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse integer: '%s'"), shares);
> + return -1;
> + }
> def->cputune.sharesSpecified = true;
> }
>
> if (virConfGetValueString(properties, "lxc.cgroup.cpu.cfs_quota_us",
> - &value) > 0) {
> - if (virStrToLong_ll(value, NULL, 10, &def->cputune.quota) < 0)
> - goto error;
> + "a) > 0) {
> + if (virStrToLong_ll(quota, NULL, 10, &def->cputune.quota) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse integer: '%s'"), quota);
> + return -1;
> + }
> }
>
> if (virConfGetValueString(properties, "lxc.cgroup.cpu.cfs_period_us",
> - &value) > 0) {
> - if (virStrToLong_ull(value, NULL, 10, &def->cputune.period) < 0)
> - goto error;
> + &period) > 0) {
> + if (virStrToLong_ull(period, NULL, 10, &def->cputune.period) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to parse integer: '%s'"), period);
> + return -1;
> + }
> }
>
> return 0;
> -
> - error:
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("failed to parse integer: '%s'"), value);
> - return -1;
I actually prefer the error label here, because we don't have to duplicate the
same error message at multiple spots.
> }
>
> static int
> lxcSetCpusetTune(virDomainDefPtr def, virConfPtr properties)
> {
> - char *value = NULL;
> + VIR_AUTOFREE(char *) cpus = NULL;
> + VIR_AUTOFREE(char *) mems = NULL;
> virBitmapPtr nodeset = NULL;
>
> if (virConfGetValueString(properties, "lxc.cgroup.cpuset.cpus",
> - &value) > 0) {
same comments apply here too...additionally (just a note out of sheer rant), we
should do something about the error within virBitmapParse, since that one isn't
going to help literally anyone, so there's no way for the user to know why
their XML failed to parse.
> - if (virBitmapParse(value, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
> + &cpus) > 0) {
> + if (virBitmapParse(cpus, &def->cpumask, VIR_DOMAIN_CPUMASK_LEN) < 0)
> return -1;
> def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC;
> }
>
> if (virConfGetValueString(properties, "lxc.cgroup.cpuset.mems",
> - &value) > 0) {
> - if (virBitmapParse(value, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
> + &mems) > 0) {
> + if (virBitmapParse(mems, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0)
> return -1;
> if (virDomainNumatuneSet(def->numa,
> def->placement_mode ==
> @@ -949,7 +964,7 @@ lxcBlkioDeviceWalkCallback(const char *name, virConfValuePtr value, void *data)
> static int
> lxcSetBlkioTune(virDomainDefPtr def, virConfPtr properties)
> {
> - char *value = NULL;
> + VIR_AUTOFREE(char *) value = NULL;
>
> if (virConfGetValueString(properties, "lxc.cgroup.blkio.weight",
> &value) > 0) {
the comments further onward would be simply the same, you get the picture, but
those are just tiny nitpicks based on personal preference I'd say (except for
copying the same error messages all over again), with that addressed:
Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
