Re: [libvirt] [PATCH 05/10] util: resctrl: refactoring some functions

On 08/27/2018 07:23 AM, Wang Huaqiang wrote: Yikes, 3 or more patches in one. Let's use @rootpath instead of @root Take it slowly - round 1, convert virResctrlAllocDeterminePath into using virResctrlDeterminePath, but don't add the @parentpath yet since it's not "introduced" until later. I'd prefer to see the same argument order as being printed too... The indent is off here w/ @id - need another space. Still is this a programming error or something else? Tough to tell since you're adding multiple things at one time. and this is just the initial case... These are additional cases added later on, but used in this patch, so they need to "wait" to be added until we see "where" they come from. Seems to me rather than passing &alloc->path, this function could return @path and the caller then be able to "handle" that. For the "first pass" before @root and @parentpath are added, using: ignore_value(virAsprintf(&path, "%s/%s-%s", rootpath, prefix, id)); return path; Thus this becomes: if (!(alloc->path = virResctrlDeterminePath(SYSFS_RESCTRL_PATH, machinename, alloc->id))) return -1; return 0; should be two blank lines between and this could use a comment describing what it's doing and what it's assumptions are. s/char/const char/ should be: virResctrlCreateGroupPath This would cause some sort of unknown error, but it's a caller bug isn't it? That is if @path is empty before calling in here, then we've missed some other condition, so in this instance it doesn't quite make sense. Not quite sure what this has to do with creating the GroupPath. Feels like some that should be in the caller, but I guess that depends on future usage.... I see this helper is called in the next patch by virResctrlAllocCreateMonitor which isn't used until patch9 and only called once/if virResctrlAllocCreate is successful. So it doesn't seem that calling it once for each time virResctrlAllocCreateMonitor is called is really necessary since @resctrl doesn't change. In fact, going back to qemuProcessResctrlCreate it would seem that calling virResctrlAllocCreate once for each vm->def->nresctrls would also be somewhat inefficient since caps->host.resctrl (a/k/a @resctrl) doesn't change. But moving it back there may mean needing to check if vm->def->resctrls[i]->alloc is NULL... I think perhaps some more thought needs to be placed on "efficient" code paths before adding the monitor code paths. This concept doesn't appear until the next patch, so we cannot introduce it yet. This Lock/Unlock sequence should be in the caller... and the fact that the lock should be taken documented as "expected" in the caller. In the short term, @ret probably isn't needed - return 0 or -1 directly. > + > /* This checks if the directory for the alloc exists. If not it tries to create > * it and apply appropriate alloc settings. */ > int > @@ -2116,21 +2185,11 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, > if (!alloc) > return 0; > > - if (virResctrlInfoIsEmpty(resctrl)) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("Resource control is not supported on this host")); > - return -1; > - } > - > if (virResctrlAllocDeterminePath(alloc, machinename) < 0) > return -1; If we return from this and alloc->path == NULL, there's a coding error, so I see no reason in virResctrlCreateGroupPath that we'd need to validate that (at least yet). It's a static helper and should be called only when your expected conditions are right. The call to virResctrlCreateGroupPath should come after this rather than Lock/Unlock when creating the directory and then Lock/Unlock again when writing to the file. I think it's all one autonomous operation. The next hunk is fine as long as it is a single patch. John > -int > -virResctrlAllocAddPID(virResctrlAllocPtr alloc, > - pid_t pid) > +static int > +virResctrlAddPID(char *path, > + pid_t pid) > { > char *tasks = NULL; > char *pidstr = NULL; > int ret = 0; > > - if (!alloc->path) { > + if (!path) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > - _("Cannot add pid to non-existing resctrl allocation")); > + _("Cannot add pid to non-existing resctrl group")); > return -1; > } > > - if (virAsprintf(&tasks, "%s/tasks", alloc->path) < 0) > + if (virAsprintf(&tasks, "%s/tasks", path) < 0) > return -1; > > if (virAsprintf(&pidstr, "%lld", (long long int) pid) < 0) > @@ -2207,6 +2259,14 @@ virResctrlAllocAddPID(virResctrlAllocPtr alloc, > > > int > +virResctrlAllocAddPID(virResctrlAllocPtr alloc, > + pid_t pid) > +{ > + return virResctrlAddPID(alloc->path, pid); > + > +int > virResctrlAllocRemove(virResctrlAllocPtr alloc) > { > int ret = 0; >