[libvirt] [PATCH v2 1/2] util: Check if kernel-provided info is consistent with itself

[Peter Krempa]

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 at redhat.com>
> > > ---
> > >  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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180205/ff018f0d/attachment-0001.sig>