[systemd-devel] [PATCH] WIP: conf-parser: allow config_parse_iec_off to parse percentages

[Jan Synacek]

Lennart Poettering <lennart at poettering.net> writes:

> On Wed, 20.05.15 10:37, jsynacek at redhat.com (jsynacek at redhat.com) wrote:
>
>> From: Jan Synacek <jsynacek at redhat.com>
>> 
>> Allow certain configuration options to be specified as percentages. For
>> example, in journald.conf, SystemMaxUse= can now also be specified as 33%.
>> 
>> There is a slight "problem" with the patch. It parses option names to determine
>> what filesystem it should use to get the available space from. This approach
>> is probably not the rigth thing to do, but I couldn't think of a better one.
>> Another approach that came to my mind was to use the highest bit of the off_t
>> value returned by the parser to indicate if the value was a percentage, or
>> a normal value. This would require to rewrite a significant amount of code
>> which now counts on the values being definite (not percentages), and would,
>> IMHO, be hardly any improvement at all.
>> 
>> What do you think? Is there a better way to implement this functionality? Is it
>> a real problem to parse the option lvalues like that?
>
> Yes, it's ugly! Also, it's problematic since disk sizes and space
> change dynamically, hence evaluating this only when parsing is not
> enough.
>
> What about this: introduce a new type:
>
>         typedef struct SizeParameter {
>                 uint64_t value;
>                 bool relative;
>         } SizeParameter;
>
> When .relative is false, then .value is an absolute value, otherwise
> it's a relative value normalized to let's say 0x100000000 (so that
> this value equals 100%, and values below it < 100% and above it >
> 100%).

Would you mind explaining this a bit more? I'm not sure if I understand
this correctly, especially the "< 100%" and "> %100" parts. It doesn't
seem to be needed at all. You always need the info from statvfs anyway,
if the value is a percentage. If not, the value can be used as-is.

> Then add new helper calls:
>
>      int size_parameter_from_string(const char *s, SizeParameter *ret);
>      uint64 size_parameter_evaluate(SizeParameter *p, uint64_t base);
>
>
> The latter should return .value as-is if p->relative is false, and
> (base * .value) >> 32 otherwise.

Why is "base" needed here?

> THen, change the appropriate places to use SizeParameter instead of
> simple uint64_t where necessary, and use size_parameter_evaluate()
> with the data from statvfs when the actual value is required.

Maybe I totally misunderstood your suggestion:) However, I tried to
implement something similar and it turned out to be non-trivial, since
there are quite a few places that need to be rewritten and tested, plus
there is some duplicate code in coredump.c that needs the testing as
well, so it may take a while.

> Lennart

Cheers,
-- 
Jan Synacek
Software Engineer, Red Hat
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20150522/90cb3903/attachment.sig>