Permille everywhere #9484
Permille everywhere #9484
Conversation
src/shared/bus-unit-util.c
Outdated
|
|
||
| n = strjoina(field, "Scale"); | ||
| r = sd_bus_message_append(m, "(sv)", n, "u", (uint32_t) (((uint64_t) UINT32_MAX * r) / 100U)); | ||
| r = sd_bus_message_append(m, "(sv)", n, "u", (uint32_t) (((uint64_t) r * (uint64_t) UINT32_MAX) / UINT64_C(1000))); |
There was a problem hiding this comment.
All that casting is not necessary. The shorter operand is always up-cast to the longer type automatically, and I don't think there's any reason to duplicate what the compiler will do anyway.
There was a problem hiding this comment.
the casting is necessary to some point. we want to do the multiplication/division in 64bit, and we want to pass 32bit value (and as this is varargs we need to be careful to put the right type on the stack).
But you are right to some point, this would suffice too, I guess:
(uint32_t) (((uint64_t) r * UINT32_MAX) / 1000)There is one reason I wrote this the way I did though: i find the C signed/unsigned propagation rules quite counter-intuitive as it's the "unsigned" that wins, not the "signed" if you multiply/divide signed with unsigned. In the syntax I chose this C behaviour is irrelevant, as we never multiply/divide two values of different signedness with each other.
There was a problem hiding this comment.
So I think
(uint32_t) (((uint64_t) r * UINT32_MAX) / 1000)
is nice… That's more-or-less what I was expecting.
(I'm complaining about the casts because it's just too easy to add casts everywhere, making the interesting bits harder to read.)
src/shared/bus-unit-util.c
Outdated
| r = parse_percent_unbounded(eq); | ||
| r = parse_permille_unbounded(eq); | ||
| if (r <= 0) { | ||
| log_error_errno(r, "CPU quota '%s' invalid.", eq); |
There was a problem hiding this comment.
log_error_errno(0, ...) is fishy.
src/shared/bus-unit-util.c
Outdated
| } | ||
|
|
||
| r = sd_bus_message_append(m, "(sv)", "CPUQuotaPerSecUSec", "t", (usec_t) r * USEC_PER_SEC / 100U); | ||
| r = sd_bus_message_append(m, "(sv)", "CPUQuotaPerSecUSec", "t", (((uint64_t) r * (uint64_t) USEC_PER_SEC) / UINT64_C(1000))); |
There was a problem hiding this comment.
Same here, just ensuring one of the variables is uint64_t is enough.
0102516 to
07e0c66
Compare
|
I force pushed a new version, with the suggested changes addressed. PTAL |
07e0c66 to
033e0a3
Compare
If 'v' is negative, it's wrong to add the decimal to it, as we'd actually need to subtract it in this case. But given that we don't want to allow negative vaues anyway, simply check earlier whether what we have parsed so far was negative, and react to that before adding the decimal to it.
…e place to permille We so far had various placed we'd parse percentages with parse_percent(). Let's make them use parse_permille() instead, which is downward compatible (as it also parses percent values), and increases the granularity a bit. Given that on the wire we usually normalize relative specifications to something like UINT32_MAX anyway changing from base-100 to base-1000 calculations can be done easily without breaking compat. This commit doesn't document this change in the man pages. While allowing more precise specifcations permille is not as commonly understood as perent I guess, hence let's keep this out of the docs for now.
033e0a3 to
f806dfd
Compare
let's allow parsing permille wherever we can