Re: [libvirt] [PATCH V3] util: Add more virsysfs functions for handling resctrl sysfs
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.
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
Attachments:
- signature.asc (application/pgp-signature — 833 bytes)