4:01 p.m.
On Wed, Nov 15, 2017 at 01:17:56PM -0500, John Ferlan wrote:
[...]
>>
>> 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.
I may miss some when amending the commits as well, even though I'm going line by
line and adjusting it as I go. I'll just put 2 everywhere.
>>> +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.
in qemu the ProcessStop is called always precisely for the reason so that we
have one place to clean up stuff.
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
I should check that the daemon is running as privileged.
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"...
It's not a normal filesystem, it behaves similarly to cgroups and it's theonly
kernel interface for this. I cannot create the files where I please.
If not should the rmdir(alloc->path) be a
virDeleteTree(alloc->path)
since you're creating "schemata" and "tasks"
I'm not, when you create the directory the files are "just there", like
cgroup
dirs. You cannot virFileDeleteTree because you'll get error on removing any
file.
John
[...]