12:17 p.m.
[...]
>
> Two blank lines (multiple instances in new functions)
>
I tried keeping this one more organized, two lines between groups of
functions, one line between functions in the same group. But I can do
two everywhere, the fact that I don't fully agree is irrelevant
(unfortunately), but I'd rather get this in instead of arguing over
the amount of whitespace =D
I guess I'm just going by other reviews I've done and received where the
2 lines was requested for anything "new"... Not everyone follows it and
I'm sure I've missed a few along the way.
>> +static int virResctrlAllocOnceInit(void)
>
> static int
> virResctrlAllocOnceInit(void)
>
Oh, missed this one.
>> +{
>> + if (!(virResctrlAllocClass = virClassNew(virClassForObject(),
>> + "virResctrlAlloc",
>> + sizeof(virResctrlAlloc),
>> + virResctrlAllocDispose)))
>> + return -1;
>> +
>> + return 0;
>> +}
>> +
>> +VIR_ONCE_GLOBAL_INIT(virResctrlAlloc)
>> +
>> +static virResctrlAllocPtr
>> +virResctrlAllocNew(void)
>> +{
>> + if (virResctrlAllocInitialize() < 0)
>> + return NULL;
>> +
>> + return virObjectNew(virResctrlAllocClass);
>> +}
>> +
>> +
>> +/* Common functions */
>> +static int
>> +virResctrlLockInternal(int op)
>> +{
>> + int fd = open(SYSFS_RESCTRL_PATH, O_DIRECTORY | O_CLOEXEC);
>> +
>> + if (fd < 0) {
>> + virReportSystemError(errno, "%s", _("Cannot open
resctrlfs"));
>> + return -1;
>> + }
>> +
>> + if (flock(fd, op) < 0) {
>
> So only ever used on a local file system right? Linux file locking
> functions are confounding...
>
Yes, only local filesystem.
> Why not use virFile{Lock|Unlock}?
>
Because that uses fcnlt(2) which is different lock which might not
interactl with flock(2), so all programs working with resctrlfs must use
the same type of lock. And resctrlfs should specifically use flock(2)
according to the docs:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/x86/intel_rdt_ui.txt
[...]
>> + if (fd == -1)
>> + return 0;
>> +
>> + /* The lock gets unlocked by closing the fd, which we need to do
>> anyway in
>> + * order to clean up properly */
>> + if (VIR_CLOSE(fd) < 0) {
>> + virReportSystemError(errno, "%s", _("Cannot close
resctrlfs"));
>> +
>> + /* Trying to save the already broken */
>
> So if close unlocks too, then why the unlock?
>
Only if the close failed, I figured we might as well try to safe the
already broken, right? I can remove it if you want.
oh right - no, it's fine here. "Eye" think I missed the on failure part!
>> + if (flock(fd, LOCK_UN) < 0)
>> + virReportSystemError(errno, "%s", _("Cannot unlock
>> resctrlfs"));
>> + return -1;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +
[...]
>> +/* This checks if the directory for the alloc exists. If
not it
>> tries to create
>> + * it and apply appropriate alloc settings. */
>> +int
>> +virResctrlAllocCreate(virResctrlInfoPtr r_info,
>> + virResctrlAllocPtr alloc,
>> + const char *drivername,
>> + const char *machinename)
>> +{
>> + char *alloc_path = NULL;
>> + char *schemata_path = NULL;
>> + bool dir_created = false;
>> + char *alloc_str = NULL;
>> + int ret = -1;
>> + int lockfd = -1;
>> +
>> + if (!alloc)
>> + return 0;
>> +
>> + if (!alloc->path &&
>> + virAsprintf(&alloc->path, "%s/%s-%s-%s",
>> + SYSFS_RESCTRL_PATH, drivername, machinename,
>> alloc->id) < 0)
>
> This is being created in /sys/fs... and theoretically nothing will
> change for @drivername and @machinename
>
>> + return -1;
>> +
>> + /* Check if this allocation was already created */
>> + if (virFileIsDir(alloc->path)) {
>> + VIR_FREE(alloc_path);
>
> dead code ;-) "alloc_path" is never allocated...
>
s/alloc_path/alloc->path/ =)
> Any concern over the guest being killed without running through
> virResctrlAllocRemove and the rmdir?
>
Yes, where do you see that? The remove will be done in
qemuProcessStop(). I can remove this one puny directory here, but
proper cleanup needs to be done for all anyway.
No where in particular - there's so much code to wade through and
attempt to remember. I do see the call for Stop - just trying to think
if there was some other way for guest death that could cause problems.
The only other thought I had along the lines here was writing into
/sys/fs - not just the privilege but the size/number of files being
created in /sys, but perhaps other things distracted those (beer?)
thoughts... I was going to ask if output such as this should be
something like ->libDir which could be deleted "for free"...
If not should the rmdir(alloc->path) be a virDeleteTree(alloc->path)
since you're creating "schemata" and "tasks"
John
[...]