[...]
>>
>> 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
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
