[libvirt] [PATCH V3] util: Add more virsysfs functions for handling resctrl sysfs

[Martin Kletzander]

On Mon, Apr 03, 2017 at 09:29:37AM +0200, Erik Skultety wrote:
>On Fri, Mar 31, 2017 at 12:59:33PM +0200, Martin Kletzander wrote:
>> On Fri, Mar 31, 2017 at 12:28:26PM +0200, Erik Skultety wrote:
>> > >  #define VIR_SYSFS_VALUE_MAXLEN 8192
>> > >  #define SYSFS_SYSTEM_PATH "/sys/devices/system"
>> > > +#define SYSFS_RESCTRL_PATH "/sys/fs/resctrl"
>> > >
>> > >  static const char *sysfs_system_path = SYSFS_SYSTEM_PATH;
>> > > +static const char *sysfs_resctrl_path = SYSFS_RESCTRL_PATH;
>> > >
>> > >
>> > >  void virSysfsSetSystemPath(const char *path)
>> > > @@ -55,6 +58,20 @@ virSysfsGetSystemPath(void)
>> > >      return sysfs_system_path;
>> > >  }
>> > >
>> > > +void virSysfsSetResctrlPath(const char *path)
>> > > +{
>> > > +    if (path)
>> > > +        sysfs_resctrl_path  = path;
>> > > +    else
>> > > +        sysfs_resctrl_path = SYSFS_RESCTRL_PATH;
>> > > +}
>> > > +
>> > > +const char *
>> > > +virSysfsGetResctrlPath(void)
>> > > +{
>> > > +    return sysfs_resctrl_path;
>> > > +}
>> > > +
>> >
>> > NACK
>> >
>> > This leads to an unnecessary code duplication (applies for most of the
>> > functions introduced by this patch). Instead, virsysfs should be made generic
>> > enough so it could be reused by any module doing sysfs related tasks, like for
>> > example the recently added mediated device framework (otherwise a new
>> > sysfs_foo_path = "/sys/bus/mdev/devices/" would need to be created as well).
>> >
>>
>> I was thinking about this, but let's discuss how to select the proper
>> line between the amount of code duplication (which I didn't like even
>> in my series) and the generality of the code (which at the end leads to
>> worse code in callers).
>>
>> Adding new set of functions for each path prefix is gross, I agree, but
>> how else could we approach this?  One of the ideas would be to have a
>> function that registers "path overrides", but that would lead to
>> unnecessary code in production where there are no tests involved.
>> Another approach is to just set the "/sys" path differently, but that
>> would mean we have to have bigger directory structures in the tests.
>> Yet another approach is to ditch virsysfs altogether and just use
>> virFile* functions.  We can mock open() and others in tests anyway
>> (like, for example, vircgrouptest does even for the sysfs system paths).
>> However, if I look at the code in the caller functions, the last
>> approach would, over time, end up duplicating more code than we do
>> currently in virsysfs.  Also, even though this is highly subjective, the
>> callers are very easy to read and understand.  More wrappers, if not
>> overused, of course, lead to cleaner codebase proportionally to the
>> codebase size.
>>
>
>The thing with code duplication is very subjective, while I say that copying
>function bodies and enclosing them with a different name, then exporting the
>symbol via a header and also adding it to the private syms is the kind of
>duplication I'd like to avoid, I understand you can say the same about my idea
>(yet to come) about having this in every caller:
>
>...
>if (!virAsprintf(path, "%s/%s", sysfs_prefix, attr))
>    goto cleanup;
>
>if (virSysfsRead[String,Int,Whatever] < 0)
>    goto cleanup;
>...
>
>So in my opinion, this kind of duplication is more acceptable than having a ton
>of symbols exporting the same functionality (aka function bodies). Now, to the
>idea, to deal with the prefixes, each of the util/virmodule.c would declare a
>string constant representing the path prefix, like virpci does with PCI_SYSFS
>and mdev with MDEV_SYSFS_DEVICES, then the burden of constructing the path would
>be on the caller (as prefaced above). I must admit that I haven't looked at the
>tests, but I'm afraid you can't get both - generic functions without introducing
>any duplicates in the code. But as we spoke privately, I think the concept of
>mocking can be reused to our liking with the sysfs accesses. So, looking at the
>tests, we will need especially need to figure out how to deal with the amount of
>files created/present under virhostcpudata. You had a specific idea in mind
>about this, so feel free to share it here.
>

Yeah, there's always pros and cons to any solution and since this is
pretty subjective, I'd opted for the one I liked the most as I feel like
code duplication of *very understandable* parts is better because it
speeds up reading the code for its callers.  Anyway, I had an idea how
to reach pretty good compromise.  I'll construct all the paths in the
callers, there will be no path masking in a global variable.  So only
virFileRead* will be used.  We might do like a few super wrappers with
various number of arguments, for example:

  virFileReadUint(&temp_uint, "/sys/devices/system/cpu/cpu%d/online", cpu_num)

And that'd be it.  I was also thinking how not to do one wrapper per
type, but I don't think that's possible to do easily with C now.
However we can have virFileReadObject() that would call a function on a
virObject and for that we need to add interfaces to our classes for
which I have an idea how to do as well, but I'm getting too tangled up
in that.

And I think I have an idea how to do all the testing of this only using
mocks in tests, without any effect on the production part of the code.
I'll try to do a proof of concept and we'll see how that goes.

>Erik
>
>> I'm not saying that what I did was the best approach, but please be
>> constructive.  If we don't have any better solutions for now, we can go
>> with one that's Good Enough™ and refactor it later when we figure out
>> how to approach it.  I'm open to suggestions and I know Eli is as well.
>>
>> Martin
>
>
>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170403/d933ccc7/attachment-0001.sig>