[libvirt] [PATCHv5 05/19] util: Refactor code for determining allocation path

[John Ferlan]

On 10/10/18 9:55 AM, Wang, Huaqiang wrote:
> 
> 
>> -----Original Message-----
>> From: John Ferlan [mailto:jferlan at redhat.com]
>> Sent: Wednesday, October 10, 2018 7:09 AM
>> To: Wang, Huaqiang <huaqiang.wang at intel.com>; libvir-list at redhat.com
>> Cc: Feng, Shaohe <shaohe.feng at intel.com>; Niu, Bing <bing.niu at intel.com>;
>> Ding, Jian-feng <jian-feng.ding at intel.com>; Zang, Rui <rui.zang at intel.com>
>> Subject: Re: [libvirt] [PATCHv5 05/19] util: Refactor code for determining
>> allocation path
>>
>>
>>
>> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
>>> The code for determining resctrl allocation path could be reused for
>>> monitor. Refactor it for reusing.
>>>
>>> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
>>> ---
>>>  src/util/virresctrl.c | 33 +++++++++++++++++++++++++++------
>>>  1 file changed, 27 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index
>>> c9a79f7..03001cc 100644
>>> --- a/src/util/virresctrl.c
>>> +++ b/src/util/virresctrl.c
>>> @@ -2267,6 +2267,26 @@ virResctrlAllocAssign(virResctrlInfoPtr
>>> resctrl,  }
>>>
>>>
>>> +static char *
>>> +virResctrlDeterminePath(const char *pathparent,
>>
>> s/pathparent/parentpath
>>
> 
> OK.
> 
>>> +                        const char *prefix,
>>> +                        const char *id) {
>>> +    char *path = NULL;
>>> +
>>> +    if (!id) {
>>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> +                       _("Resctrl resource ID must be set before
>>> + creation"));
>>
>> s/Resctrl resource ID/'%s' resctrl ID/
>>
>> where %s is @parentpath
>>
>> Working in the @parentpath, then we'd know which it was Alloc or Monitor
> 
> How about changes like these?
> 
>      if (!id) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("Resctrl resource ID must be set before creation"));
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Resctrl ID must be set before determining resctrl "
> +                         "path under '%s'"),
> +                       parentpath);
>          return NULL;
>      }
> 

Seems reasonable... although instead of "path under", just go with
"parentpath='%s'"

John

> 
>>
>>> +        return NULL;
>>> +    }
>>> +
>>> +    if (virAsprintf(&path, "%s/%s-%s", pathparent, prefix, id) < 0)
>>
>> parentpath
> 
> Will be renamed.
> 
>>
>>> +        return NULL;
>>> +
>>> +    return path;
>>> +}
>>> +
>>> +
>>>  int
>>>  virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
>>>                               const char *machinename) @@ -2274,15
>>> +2294,16 @@ virResctrlAllocDeterminePath(virResctrlAllocPtr alloc,
>>>      if (!alloc)
>>>          return 0;
>>>
>>> -    if (!alloc->id) {
>>> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> -                       _("Resctrl Allocation ID must be set before creation"));
>>> +    if (alloc->path) {
>>> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>>> +                       _("Resctrl group path is expected to be
>>> + NULL"));
>>
>> Resctrl alloc group path is already set '%s'
>>
>> I think this is an internal error not an invalid arg, too
> 
> Will be changed.
> 
>      if (alloc->path) {
> -        virReportError(VIR_ERR_INVALID_ARG, "%s",
> -                       _("Resctrl group path is expected to be NULL"));
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Resctrl allocation path is already set to '%s'"),
> +                       alloc->path);
>          return -1;
>      }
> 
>>
>> John
>>
> 
> 
> Thanks for review.
> Huaqiang
> 
>>>          return -1;
>>>      }
>>>
>>> -    if (!alloc->path &&
>>> -        virAsprintf(&alloc->path, "%s/%s-%s",
>>> -                    SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0)
>>> +    alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH,
>>> +                                          machinename,
>>> +                                          alloc->id);
>>> +    if (!alloc->path)
>>>          return -1;
>>>
>>>      return 0;
>>>
>