Re: [PATCH 2/3] lxc: Resolve memory leak

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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;
>> +                              &quota) > 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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux