[libvirt] [PATCHv5 08/19] util: Add interface for creating monitor group

[Wang, Huaqiang]

On 10/11/2018 3:13 AM, John Ferlan wrote:
>
> On 10/9/18 6:30 AM, Wang Huaqiang wrote:
>> Add interface for creating the resource monitoring group according
>> to '@virResctrlMonitor->path'.
>>
>> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
>> ---
>>  src/libvirt_private.syms |  1 +
>>  src/util/virresctrl.c    | 28 ++++++++++++++++++++++++++++
>>  src/util/virresctrl.h    |  6 ++++++
>>  3 files changed, 35 insertions(+)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index e175c8b..a878083 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2681,6 +2681,7 @@ virResctrlInfoGetMonitorPrefix;
>>  virResctrlInfoMonFree;
>>  virResctrlInfoNew;
>>  virResctrlMonitorAddPID;
>> +virResctrlMonitorCreate;
>>  virResctrlMonitorDeterminePath;
>>  virResctrlMonitorNew;
>>  
>> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
>> index 8b617a6..b3d20cc 100644
>> --- a/src/util/virresctrl.c
>> +++ b/src/util/virresctrl.c
>> @@ -2475,6 +2475,7 @@ virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
>>      return virResctrlAddPID(monitor->path, pid);
>>  }
>>  
>> +
> This should have been squashed into patch6

OK. Two lines between each function.

>
>>  int
>>  virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
>>                                 const char *machinename)
>> @@ -2506,3 +2507,30 @@ virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
>>  
>>      return 0;
>>  }
>> +
>> +
>> +int
>> +virResctrlMonitorCreate(virResctrlAllocPtr alloc,
>> +                        virResctrlMonitorPtr monitor,
>> +                        const char *machinename)
>> +{
>> +    int lockfd = -1;
>> +    int ret = -1;
>> +
>> +    if (!monitor)
>> +        return 0;
>> +
>> +    monitor->alloc = virObjectRef(alloc);
> Can @alloc be NULL here? I see that the eventual caller from
> qemuProcessResctrlCreate would pass vm->def->resctrls[i]->alloc after a
> virResctrlAllocCreate, but that API can return 0 immediately "if
> (!alloc)"?

@alloc could be NULL.
In virResctrlAllocCreate, if the passed in @alloc is NULL, the virResctrlAllocCreate
function will return 0, this is not considered as an error.

Following the code invoking virResctrlMonitorCreate, if @vm->def->resctrls[i]->alloc
is NULL, then the virResctrlAllocCreate function will return 0, this is not an error,
code going on, then virResctrlMonitorCreate will be invoked if @nmonitors is not 0,
in this case, the first argument,the @alloc, of virResctrlMonitorCreate is NULL.

2613     for (i = 0; i < vm->def->nresctrls; i++) {
2614         size_t j = 0;
2615         if (virResctrlAllocCreate(caps->host.resctrl,                                
2616                                   vm->def->resctrls[i]->alloc,
2617                                   priv->machineName) < 0)
2618             goto cleanup;
2619
2620         for (j = 0; j < vm->def->resctrls[i]->nmonitors; j++) {
2621             virDomainResctrlMonDefPtr mon = NULL;
2622    
2623             mon = vm->def->resctrls[i]->monitors[j];
2624             if (virResctrlMonitorCreate(vm->def->resctrls[i]->alloc,
2625                                         mon->instance,
2626                                         priv->machineName) < 0)
2627                 goto cleanup;
2628        
2629         }
2630     } 
>
>
> Furthermore, if we Ref it here, but return -1 in the subsequent steps
> are we sure that the Unref gets done.

If in later steps error happens and returns a -1, then it will go through the resource
releasing functions by calling virResctrlMonitorDispose and virResctrlAllocDispose.
The unref of @monitor->alloc is done in virResctrlMonitorDispose (function
introduced in patch2):

 401 static void
 402 virResctrlMonitorDispose(void *obj)
 403 {
 404     virResctrlMonitorPtr monitor = obj;
 405
 406     virObjectUnref(monitor->alloc);
 407     VIR_FREE(monitor->id);
 408     VIR_FREE(monitor->path);
 409 }

>
> The one "thing" about the order of patches here is that it forces me to
> look forward to ensure decisions made in previous patches will be
> handled in the future.

Maybe combine virResctrlMonitorDispose and virResctrlMonitorCreate in one
patch? Will that make you look better for understanding how @monitor->alloc
is used.

>
>
>> +
>> +    if (virResctrlMonitorDeterminePath(monitor, machinename) < 0)
>> +        return -1;
> At least for now virResctrlMonitorDeterminePath can handle a NULL
> monitor->alloc...
>
> John

Thanks for review.
Huaqiang

>
>
>> +
>> +    lockfd = virResctrlLockWrite();
>> +    if (lockfd < 0)
>> +        return -1;
>> +
>> +    ret = virResctrlCreateGroupPath(monitor->path);
>> +
>> +    virResctrlUnlock(lockfd);
>> +    return ret;
>> +}
>> diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h
>> index 69b6b1d..1efe394 100644
>> --- a/src/util/virresctrl.h
>> +++ b/src/util/virresctrl.h
>> @@ -196,7 +196,13 @@ virResctrlMonitorNew(void);
>>  int
>>  virResctrlMonitorAddPID(virResctrlMonitorPtr monitor,
>>                          pid_t pid);
>> +
>>  int
>>  virResctrlMonitorDeterminePath(virResctrlMonitorPtr monitor,
>>                                 const char *machinename);
>> +
>> +int
>> +virResctrlMonitorCreate(virResctrlAllocPtr alloc,
>> +                        virResctrlMonitorPtr monitor,
>> +                        const char *machinename);
>>  #endif /*  __VIR_RESCTRL_H__ */
>>