[libvirt] [PATCHv7 04/18] util: Add interface to determine monitor path

[Huaqiang,Wang]

On 2018年11月06日 01:19, John Ferlan wrote:
>
> On 10/22/18 4:01 AM, Wang Huaqiang wrote:
>> Add interface for resctrl monitor to determine the path.
>>
>> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
>> ---
>>   src/libvirt_private.syms |  1 +
>>   src/util/virresctrl.c    | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   src/util/virresctrl.h    |  5 ++++-
>>   3 files changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index d2573c5..5235046 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2680,6 +2680,7 @@ virResctrlInfoGetCache;
>>   virResctrlInfoGetMonitorPrefix;
>>   virResctrlInfoMonFree;
>>   virResctrlInfoNew;
>> +virResctrlMonitorDeterminePath;
>>   virResctrlMonitorNew;
>>   
>>   
>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>> index 956aca8..1d0eb01 100644
>> --- a/src/util/virresctrl.c
>> +++ b/src/util/virresctrl.c
>> @@ -2462,3 +2462,58 @@ virResctrlMonitorNew(void)
>>   
>>       return virObjectNew(virResctrlMonitorClass);
>>   }
>> +
>> +
>> +/*
>> + * virResctrlMonitorDeterminePath
>> + *
>> + * @monitor: Pointer to a resctrl monitor
>> + * @machinename: Name string of the VM
>> + *
>> + * Determines the directory path that the underlying resctrl group will be
>> + * created with.
>> + *
>> + * A monitor represents a directory under resource control file system,
>> + * its directory path could be the same path as @monitor->alloc, could be a
>> + * path of directory under 'mon_groups' of @monitor->alloc, or a path of
>> + * directory under '/sys/fs/resctrl/mon_groups' if @monitor->alloc is NULL.
>> + *
>> + * Returns 0 on success, -1 on error.
>> + */
>> +int
>> +virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
>> +                               const char *machinename)
>> +{
>> +    VIR_AUTOFREE(char *) parentpath = NULL;
>> +
>> +    if (!monitor) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("Invalid resctrl monitor"));
>> +        return -1;
>> +    }
>> +
>> +    if (monitor->path) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("Resctrl monitor path is already set to '%s'"),
>> +                       monitor->path);
>> +        return -1;
>> +    }
>> +
>> +    if (monitor->id && monitor->alloc && monitor->alloc->id) {
>> +        if (STREQ(monitor->id, monitor->alloc->id)) {
>> +            if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
>> +                return -1;
>> +            return 0;
>> +        }
>> +    }
>> +
>> +    if (virAsprintf(&parentpath, "%s/mon_groups", monitor->alloc->path) < 0)
> Just ran the changes through Coverity - because of the above
> "monitor->alloc" check, Coverity notes right here that monitor->alloc
> could be NULL, so I think a check prior to here would be in order,

Yes. Here exists a risk that this line you mentioned could be executed at
the condition that @monitor->alloc is empty pointer, this will cause a 
crash.

> such
> as either before or after the monitor->path check:
>
>      if (!monitor->alloc) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("Missing resctrl monitor allocation"));
>          return -1;
>      }

Putting these lines before and after the monitor->path check should be 
OK for me.
(I don't find the influence made by the order. )

>
> Then the monitor->alloc check wouldn't be necessary. Thus the above
> STRDUP becomes:
>
>      if (STREQ_NULLABLE(monitor->id, monitor->alloc->id)) {
>          if (VIR_STRDUP(monitor->path, monitor->alloc->path) < 0)
>              return -1;
>          return 0;
>      }

Agree.
Thanks.

>
> Let me know where you want it.
>
> John

Thanks for review.
Huaqiang

>
>> +        return -1;
>> +
>> +    monitor->path = virResctrlDeterminePath(parentpath, machinename,
>> +                                            monitor->id);
>> +    if (!monitor->path)
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
>> index eaa077d..baae554 100644
>> --- a/src/util/virresctrl.h
>> +++ b/src/util/virresctrl.h
>> @@ -191,7 +191,10 @@ virResctrlInfoGetMonitorPrefix(virResctrlInfoPtr resctrl,
>>   typedef struct _virResctrlMonitor virResctrlMonitor;
>>   typedef virResctrlMonitor *virResctrlMonitorPtr;
>>   
>> -
>>   virResctrlMonitorPtr
>>   virResctrlMonitorNew(void);
>> +
>> +int
>> +virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
>> +                               const char *machinename);
>>   #endif /*  __VIR_RESCTRL_H__ */
>>