Pointed out during review on one or two places, but it actually appears in lot more places. So let's be consistent. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/qemu/qemu_process.c | 2 +- src/util/virresctrl.c | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3a697de037e1..0577f4c35d08 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5936,7 +5936,7 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuProcessSetupEmulator(vm) < 0) goto cleanup; - VIR_DEBUG("Setting up resctrlfs"); + VIR_DEBUG("Setting up resctrl"); if (qemuProcessResctrlCreate(driver, vm) < 0) goto cleanup; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 03218a481dc0..a1d09c547e45 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -76,7 +76,7 @@ struct _virResctrlInfoPerType { unsigned int max_cache_id; /* In order to be self-sufficient we need size information per cache. - * Funnily enough, one of the outcomes of the resctrlfs design is that it + * Funnily enough, one of the outcomes of the resctrl design is that it * does not account for different sizes per cache on the same level. So * for the sake of easiness, let's copy that, for now. */ unsigned long long size; @@ -302,12 +302,12 @@ virResctrlLockInternal(int op) int fd = open(SYSFS_RESCTRL_PATH, O_DIRECTORY | O_CLOEXEC); if (fd < 0) { - virReportSystemError(errno, "%s", _("Cannot open resctrlfs")); + virReportSystemError(errno, "%s", _("Cannot open resctrl")); return -1; } if (flock(fd, op) < 0) { - virReportSystemError(errno, "%s", _("Cannot lock resctrlfs")); + virReportSystemError(errno, "%s", _("Cannot lock resctrl")); VIR_FORCE_CLOSE(fd); return -1; } @@ -328,7 +328,7 @@ static inline int virResctrlLockWrite(void) { virReportSystemError(ENOSYS, "%s", - _("resctrlfs not supported on this platform")); + _("resctrl not supported on this platform")); return -1; } @@ -347,11 +347,11 @@ virResctrlUnlock(int fd) /* 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")); + virReportSystemError(errno, "%s", _("Cannot close resctrl")); /* Trying to save the already broken */ if (flock(fd, LOCK_UN) < 0) - virReportSystemError(errno, "%s", _("Cannot unlock resctrlfs")); + virReportSystemError(errno, "%s", _("Cannot unlock resctrl")); return -1; } #endif /* ! __linux__ */ @@ -486,7 +486,7 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) if (i_level->types[type]) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Duplicate cache type in resctrlfs for level %u"), + _("Duplicate cache type in resctrl for level %u"), level); goto cleanup; } -- 2.16.1

Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/util/virresctrl.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 89b1382b6857..684cb22fd8a2 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -392,7 +392,6 @@ int virResctrlGetInfo(virResctrlInfoPtr resctrl) { DIR *dirp = NULL; - char *info_path = NULL; char *endptr = NULL; char *tmp_str = NULL; int ret = -1; @@ -507,7 +506,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) ret = 0; cleanup: VIR_DIR_CLOSE(dirp); - VIR_FREE(info_path); if (i_type) VIR_FREE(i_type->cbm_mask); VIR_FREE(i_type); -- 2.16.1

We are skipping non-directories under /sys/fs/resctrl/info/ since those are not interesting for us. However in tests it can sometimes happen that ent->d_type is 0 instead of 4 (DT_DIR) for directories. I've seen it fail on two machines. Different machines, different systems, I cannot reproduce it even using the same setup. So one of the ways how to work around this is call stat() on it. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/util/virresctrl.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 684cb22fd8a2..12dda76f9d33 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -409,11 +409,32 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) } while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { + struct stat st; + char *path = NULL; + VIR_DEBUG("Parsing info type '%s'", ent->d_name); + if (virAsprintf(&path, SYSFS_RESCTRL_PATH "/info/%s", ent->d_name) < 0) + continue; + + if (stat(path, &st) < 0) { + virReportSystemError(errno, _("Cannot stat '%s'"), path); + VIR_FREE(path); + continue; + } + VIR_FREE(path); + + /* + * So this doesn't work on some machines when we're mocking syscalls in tests + if (ent->d_type != DT_DIR) continue; + But the following does, for some reason. + */ + if ((st.st_mode & S_IFMT) != S_IFDIR) + continue; + if (ent->d_name[0] != 'L') continue; -- 2.16.1

We are skipping non-directories under /sys/fs/resctrl/(info/) since those are not interesting for us. However in tests it can sometimes happen that ent->d_type is 0 instead of 4 (DT_DIR) for directories. I've seen it fail on two machines. Different machines, different systems, I cannot reproduce it even using the same setup. So one of the ways how to work around this is call stat() on it. The other one is not checking if it is a directory since we'll find out eventually when we want to read some files underneath it. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/util/virresctrl.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 684cb22fd8a2..ad4294c26b1e 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -410,10 +410,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH "/info")) > 0) { VIR_DEBUG("Parsing info type '%s'", ent->d_name); - - if (ent->d_type != DT_DIR) - continue; - if (ent->d_name[0] != 'L') continue; @@ -436,19 +432,28 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) rv = virFileReadValueUint(&i_type->control.max_allocation, SYSFS_RESCTRL_PATH "/info/%s/num_closids", ent->d_name); - if (rv == -2) - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot get num_closids from resctrl cache info")); - if (rv < 0) + if (rv == -2) { + /* The file doesn't exist, so it's unusable for us, + * but we can scan further */ + VIR_WARN("The path '" SYSFS_RESCTRL_PATH "/info/%s/num_closids' " + "does not exist", + ent->d_name); + } else if (rv < 0) { + /* Other failures are fatal, so just quit */ goto cleanup; + } rv = virFileReadValueString(&i_type->cbm_mask, SYSFS_RESCTRL_PATH "/info/%s/cbm_mask", ent->d_name); - if (rv == -2) + if (rv == -2) { + /* If the previous file exists, so should this one. Hence -2 is + * fatal in this case as well - the kernel interface might've + * changed too much or something else is wrong. */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot get cbm_mask from resctrl cache info")); + } if (rv < 0) goto cleanup; @@ -1169,9 +1174,6 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) goto error; while ((rv = virDirRead(dirp, &ent, SYSFS_RESCTRL_PATH)) > 0) { - if (ent->d_type != DT_DIR) - continue; - if (STREQ(ent->d_name, "info")) continue; -- 2.16.1

On Mon, Jan 29, 2018 at 05:54:03PM +0100, Ján Tomko wrote: No, it really is fatal, it's just that it errors out in the next condition, which looks ugly, I'll fix the comment at least, since changing the code would need changes in later parts as well in order to stay consistent. Thanks for the review.