On Mon, Feb 05, 2018 at 10:37:25 +0100, Martin Kletzander wrote: > On Fri, Feb 02, 2018 at 06:02:22PM +0100, Peter Krempa wrote: > > On Fri, Feb 02, 2018 at 15:27:21 +0100, Martin Kletzander wrote: > > > Just in case someone re-mounted /sys/fs/resctrl with different mount > > > options (cdp), add a check here. > > > > > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1540780 > > > > > > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx> > > > --- > > > src/util/virresctrl.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c > > > index ef388757a704..6860e86e649d 100644 > > > --- a/src/util/virresctrl.c > > > +++ b/src/util/virresctrl.c > > > @@ -941,6 +941,17 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl, > > > if (!mask) > > > return -1; > > > > > > + if (!resctrl || > > > + level >= resctrl->nlevels || > > > + !resctrl->levels[level] || > > > + !resctrl->levels[level]->types[type]) { > > > > The only caller of this function checks the range of type by converting > > it from string with 'virResctrlTypeFromString' but the type in this > > function is 'virCacheType' and you use 'virCacheTypeToString'. > > > > Given the inconsistency and the fact that C will not validate the passed > > type in this case you should also validate that 'type' has the correct > > range. > > > > ACK with that check added. > > There are three "inconsistent" types, but they all use VIR_CACHE_TYPE_LAST in > their VIR_ENUM_IMPL: > > - virResctrl - That's the naming we need to use when writing into resctrl schemata file > > - virCache - That's the naming that was decide upstream that we should use in our XML > > - virCacheKernel - That's what kernel uses when we read from cache info from sysfs > > I have no problem with adding one extra check here, but I think this is taken > care of by the fact that they all use the same enum for the implementation. I > can just extract the const char * fromt he error message before the check and > just add `str != NULL` in the condition. I'll leave that choice up to you. I > agree the code is not as nice as it could be and I'm already working on at least > the minimal refactoring that needs tobe done, but in the meantime I'd like to > have this working at least. The number of workarounds we need to make due to > IMNSHO poor implementation of resctrl in kernel is sickening^Wtiring let's say. > > Just so we don't misunderstand each other, I'm not against any changes to that > code. Feel free to suggest any other things that might be made better, I'm all > for making this nicer. It's just that most of the time I couldn't find any > nicer way to do some of these _suboptimal_ things. Hmm, okay fair enough. Maybe just add a comment to the enum implementations that they should all be terminated via VIR_CACHE_TYPE_LAST since code depends on it. It can definitely be separate in this case. ACK to this patch as-is.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
