On 09/21/2018 04:43 AM, Erik Skultety wrote:
> 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.
>
In this instance, how?
>>
>> if (virConfGetValueString(properties,
>> "lxc.cgroup.memory.limit_in_bytes",
>> - &value) > 0) {
^^^ hard_limit instead of value, 4 spaces in for the "if"... This
repeats for each one. They're all top level.
>> - 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...
>
Generally speaking - I agree about the common error label; however, in
this case without looking at the lxcConvertSize because the previous
code was:
- error:
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to parse integer: '%s'"), value);
and @value was being replaced by 3 new variables each unique, using a
common label was not viable.
So there's really two patches here. The first one should remove those
error: labels because the lxcConvertSize already generates one. Then
the second patch would be to convert to VIR_AUTOFREE.
>> + 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.
>
Oh, so you want a rewrite of the logic to call some local that will
return some value, but then we create a local for handling when @value
is a virStrToLong_i, virStrToLong_ui, virStrToLong_ull, or
virBitmapParse. That would mean another pre-patch.
Or maybe I'm being dense and just missing your point
>> - 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.
>
Again, maybe I'm missing something subtle in your comment.
>> }
>>
>> 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.
>
Oh so you want me to step into the cpuset="" argument ;-) No thanks!
>> - 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:
Nope, the picture wasn't clear, but I can/should check my coffee to see
if it's strong enough.
John
>
> Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
