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

[Lennart Poettering]

On Fri, 22.05.15 12:30, Jan Synacek (jsyna...@redhat.com) wrote:

> > 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 0x1 (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.

Well, basically what I am saying is that you should not store a
*percentage*, but a value relative to UINT32_MAX+1.

e.g.0% would map to value=0
   50% would map to (UINT32_MAX+1)/2
  100% would map to UINT32_MAX+1
  120% would map to ((UINT32_MAX+1)*6)/5
  200% would map to (UINT32_MAX+1)*2

Or to put this differently, consider .value a fixed point number with
32bit before and 32bit after the point...

The point is simply that 0..100 is a weird range on computers, that
calculate in binary. Hence expose 0..100 in the UI, but internally
calculate with something more appropriate.
 
> > 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?

"base" is the value you get from statfs.

The idea is basically that instead of storing just an absolute value,
you store either a relative or an absolute value, and remember which
one in a boolean. And then, when you actually need to evaluate things
you pass the statvfs data in and it will translate the relative value
to an absolute value -- but only at the time you actually need it.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

[Jan Synacek]

Lennart Poettering  writes:

> On Wed, 20.05.15 10:37, jsyna...@redhat.com (jsyna...@redhat.com) wrote:
>
>> From: Jan Synacek 
>> 
>> 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 0x1 (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


signature.asc
Description: PGP signature
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

[Lennart Poettering]

On Wed, 20.05.15 10:37, jsyna...@redhat.com (jsyna...@redhat.com) wrote:

> From: Jan Synacek 
> 
> 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 0x1 (so that
this value equals 100%, and values below it < 100% and above it >
100%).

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.

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.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

[systemd github import bot]

Patchset imported to github.
Pull request:


--
Generated by https://github.com/haraldh/mail2git
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

[jsynacek]

From: Jan Synacek 

---
 src/shared/conf-parser.c | 42 ++
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/src/shared/conf-parser.c b/src/shared/conf-parser.c
index 2c85515..d7d9aa4 100644
--- a/src/shared/conf-parser.c
+++ b/src/shared/conf-parser.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "conf-parser.h"
 #include "conf-files.h"
@@ -517,6 +518,37 @@ int config_parse_si_size(const char* unit,
 return 0;
 }
 
+static int parse_percent(const char *lvalue, const char *rvalue, off_t *bytes) 
{
+char *p, *e;
+off_t percent;
+struct statvfs ss;
+
+p = endswith(rvalue, "%");
+if (!p)
+return -ERANGE;
+
+percent = strtoll(rvalue, &e, 10);
+if (*e != '%' || e != p)
+return -ERANGE;
+
+if (percent > 100)
+return -ERANGE;
+
+if (!startswith(lvalue, "System") && !startswith(lvalue, "Runtime")) {
+if (statvfs("/var/lib/systemd/coredump", &ss) < 0)
+return -errno;
+} else if (startswith(lvalue, "System")) {
+if (statvfs("/var/log/journal", &ss) < 0)
+return -errno;
+} else
+if (statvfs("/run/log", &ss) < 0)
+return -errno;
+
+*bytes = percent * 0.01 * ss.f_bsize * ss.f_bavail;
+
+return 0;
+}
+
 int config_parse_iec_off(const char* unit,
const char *filename,
unsigned line,
@@ -538,10 +570,12 @@ int config_parse_iec_off(const char* unit,
 
 assert_cc(sizeof(off_t) == sizeof(uint64_t));
 
-r = parse_size(rvalue, 1024, bytes);
-if (r < 0)
-log_syntax(unit, LOG_ERR, filename, line, -r, "Failed to parse 
size value, ignoring: %s", rvalue);
-
+r = parse_percent(lvalue, rvalue, bytes);
+if (r < 0) {
+r = parse_size(rvalue, 1024, bytes);
+if (r < 0)
+log_syntax(unit, LOG_ERR, filename, line, -r, "Failed 
to parse size value, ignoring: %s", rvalue);
+}
 return 0;
 }
 
-- 
2.1.0

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

[jsynacek]

From: Jan Synacek 

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?

Jan Synacek (1):
  WIP: conf-parser: allow config_parse_iec_off to parse percentages

 src/shared/conf-parser.c | 42 ++
 1 file changed, 38 insertions(+), 4 deletions(-)

-- 
2.1.0

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel