Re: [libvirt PATCH 1/3] vircgroup: introduce virCgroupGetInode function

On 8/4/21 10:22 AM, Pavel Hrdina wrote: > On Wed, Aug 04, 2021 at 10:06:17AM +0200, Michal Prívozník wrote: >> On 8/3/21 4:29 PM, Pavel Hrdina wrote: >>> For new feature Fibre Channel VMID we will need to get inode of the >>> VM root cgroup as it is used in the new kernel API together with VMID. >>> >>> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; >>> --- >>> src/libvirt_private.syms | 1 + >>> src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++++++++++ >>> src/util/vircgroup.h | 2 ++ >>> 3 files changed, 40 insertions(+) >>> >>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms >>> index 6961cdb137..d8451fcfff 100644 >>> --- a/src/libvirt_private.syms >>> +++ b/src/libvirt_private.syms >>> @@ -1920,6 +1920,7 @@ virCgroupGetCpuShares; >>> virCgroupGetDevicePermsString; >>> virCgroupGetDomainTotalCpuStats; >>> virCgroupGetFreezerState; >>> +virCgroupGetInode; >>> virCgroupGetMemoryHardLimit; >>> virCgroupGetMemorySoftLimit; >>> virCgroupGetMemoryStat; >>> diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c >>> index 1b3b28342e..ea702e7b63 100644 >>> --- a/src/util/vircgroup.c >>> +++ b/src/util/vircgroup.c >>> @@ -3973,3 +3973,40 @@ virCgroupGetCpuPeriodQuota(virCgroup *cgroup, unsigned long long *period, >>> >>> return 0; >>> } >>> + >>> + >>> +/** >>> + * virCgroupGetInode: >>> + * >>> + * @cgroup: the cgroup to get inode for >>> + * >>> + * Get the @cgroup inode and return its value to the caller. >>> + * >>> + * Returns inode on success, -1 on error with error message reported. >>> + */ >>> +int >>> +virCgroupGetInode(virCgroup *cgroup) >>> +{ >>> + VIR_AUTOCLOSE fd = 0; >>> + struct stat st; >>> + int controller = virCgroupGetAnyController(cgroup); >>> + g_autofree char *path = NULL; >>> + >>> + if (controller < 0) >>> + return -1; >>> + >>> + if (virCgroupPathOfController(cgroup, controller, "", &path) < 0) >>> + return -1; >>> + >>> + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) { >>> + virReportSystemError(errno, _("failed to open cgroup '%s'"), path); >>> + return -1; >>> + } >> >> Is the open() necessary? Why isn't plain stat() enough? > > Good question :) I was lazy so I looked for first similar code in > libvirt codebase and copied it. You are right, stat should be enough > as according to man page stat and fstat are identical except or the > fd vs path parameter. I'll fix it before pushing or should I send a v2? No need, just drop open() and switch to stat() before pushing. My Reviewed-by still applies.