On 08/27/2018 07:23 AM, Wang Huaqiang wrote:
Some code, in virresctrl.c, manupulating the file objects of resctrlfs could be reused for cache monitor interfaces. This patch refactor these functions for purpose of reusing code in later patch:
virResctrlAllocDeterminePath virResctrlAllocCreate virResctrlAddPID
Signed-off-by: Wang Huaqiang <huaqiang.wang@intel.com> --- src/util/virresctrl.c | 126 +++++++++++++++++++++++++++++++++++++------------- 1 file changed, 93 insertions(+), 33 deletions(-)
Yikes, 3 or more patches in one.
diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 2f6923a..b3bae6e 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -2082,25 +2082,94 @@ virResctrlAllocAssign(virResctrlInfoPtr resctrl, }
-int -virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, - const char *machinename) +static int +virResctrlDeterminePath(const char *id, + const char *root,
Let's use @rootpath instead of @root
+ const char *parentpath, + const char *prefix, + char **path)
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...
{ - if (!alloc->id) { + if (!id) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Resctrl Allocation ID must be set before creation")); + _("Resctrl resource ID must be set before creation")); return -1; }
- if (!alloc->path && - virAsprintf(&alloc->path, "%s/%s-%s", - SYSFS_RESCTRL_PATH, machinename, alloc->id) < 0) + if (*path) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Resctrl group (%s) already created, path=%s."), + id, *path);
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.
return -1; + } + + if (!parentpath && !root) { + if (virAsprintf(path, "%s/%s-%s", + SYSFS_RESCTRL_PATH, prefix, id) < 0) + return -1;
and this is just the initial case...
+ } else if (!parentpath) { + if (virAsprintf(path, "%s/%s/%s-%s", + SYSFS_RESCTRL_PATH, parentpath, prefix, id) < 0) + return -1; + } else { + if (virAsprintf(path, "%s/%s/%s-%s", + root, parentpath, prefix, id) < 0) + return -1; + }
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.
return 0;
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;
}
+int +virResctrlAllocDeterminePath(virResctrlAllocPtr alloc, + const char *machinename) +{ + return virResctrlDeterminePath(alloc->id, NULL, NULL, + machinename, &alloc->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.
+static int +virResctrlCreateGroup(virResctrlInfoPtr resctrl, + char *path)
s/char/const char/ should be: virResctrlCreateGroupPath
+{ + int ret = -1; + int lockfd = -1; + + if (!path) + return -1;
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.
+ + if (virResctrlInfoIsEmpty(resctrl)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Resource control is not supported on this host")); + return -1; + }
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.
+ + if (STREQ(path, SYSFS_RESCTRL_PATH)) + return 0;
This concept doesn't appear until the next patch, so we cannot introduce it yet.
+ + if (virFileExists(path)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Path '%s' for resctrl resource group exists"), path); + goto cleanup; + } + + lockfd = virResctrlLockWrite(); + if (lockfd < 0) + goto cleanup;
This Lock/Unlock sequence should be in the caller... and the fact that the lock should be taken documented as "expected" in the caller.
+ + if (virFileMakePath(path) < 0) { + virReportSystemError(errno, + _("Cannot create resctrl directory '%s'"), path); + goto cleanup; + } + + ret = 0; + cleanup: + virResctrlUnlock(lockfd); + return ret;
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.
- if (virFileExists(alloc->path)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Path '%s' for resctrl allocation exists"), - alloc->path); - goto cleanup; - } + if (virResctrlCreateGroup(resctrl, alloc->path) < 0) + return -1;
lockfd = virResctrlLockWrite(); if (lockfd < 0)
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.
@@ -2146,13 +2205,6 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, if (virAsprintf(&schemata_path, "%s/schemata", alloc->path) < 0) goto cleanup;
- if (virFileMakePath(alloc->path) < 0) { - virReportSystemError(errno, - _("Cannot create resctrl directory '%s'"), - alloc->path); - goto cleanup; - } - VIR_DEBUG("Writing resctrl schemata '%s' into '%s'", alloc_str, schemata_path); if (virFileWriteStr(schemata_path, alloc_str, 0) < 0) { rmdir(alloc->path); @@ -2171,21 +2223,21 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, }
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;