(... long email) On 11/17/21 19:07, Richard W.M. Jones wrote:

On Mon, Nov 15, 2021 at 02:11:01PM +0100, Laszlo Ersek wrote:

...

(2) Should nbdkit continue building on RHEL7? In the OCaml upgrade thread <https://listman.redhat.com/archives/libguestfs/2021-November/msg00095.html> I thought we practically abandoned RHEL7 for the v2v projects.

If I sound schizophrenic on RHEL 7 that'll be because I'm in two minds still about supporting it.

In this particular case I was trying to test the OCaml changes on the oldest version of OCaml I have easy access to and found that there were various problems compiling on RHEL 7. I fixed the other problems, but this fix was a bit more controversial.

...

(3) Red Hat Developer Toolset 10 (and 11 Beta) are available for RHEL7:

https://developers.redhat.com/products/developertoolset/overview

https://access.redhat.com/documentation/en-us/red_hat_developer_toolset/10/h...

https://access.redhat.com/documentation/en-us/red_hat_developer_toolset/11-b...

providing gcc 10 and 11 respectively.

We could say that building nbdkit on RHEL7 requires devtoolset.

This doesn't sound very convenient, but there's a stronger reason to keep stuff working with GCC 3/4 and that's because OpenBSD sticks with GCC 4.2 as it was the last GPLv2+ version. I think the reason OpenBSD does this is wrong, but that's their choice. (Of course they have Clang).

...

(1) I think that APIs that lie are wrong. I could contribute standard C implementations if we really think nbdkit should continue building on RHEL7. The difference with the built-ins should only be in performance, not in functionality.

I agree. If you want to have a go at standard C versions that would be great. Even unsigned addition looks very hard.

I found this lengthy paper on the subject: https://www.cs.utah.edu/~regehr/papers/overflow12.pdf

I've not checked the paper, but I can say two things at once: - "~regehr" means it's going to be great :) - signed integers are a huge complication, while at the same time, their use (or usefulness) is extremely limited for dealing with memory allocations, sizes, indexing, file offsets, and so on. I wouldn't try to do this in a type-transparent manner. I'd first revert commit b31859402d14 ("common/include/checked-overflow.h: Simplify", 2021-11-11), and then add separate functions for uint64_t and size_t. If we don't want to be 100% standard C, just make it work for gcc-4.2, then a type-generic(-ish) solution could exist using "typeof" and (perhaps) statement expressions. (Both are GCC extensions that exist in 4.2: - <https://gcc.gnu.org/onlinedocs/gcc-4.2.4/gcc/Statement-Exprs.html> - <https://gcc.gnu.org/onlinedocs/gcc-4.2.4/gcc/Typeof.html>.) To go into some details: (1) The idea is that we want the following C expression to evaluate to true, without OP overflowing first: a OP b <= max where "a", "b", "max" are all of the same unsigned integer type, and OP is either "+" or "*". (2) In case OP is "+": a + b <= max we can always subtract "b" from both sides: a <= max - b This condition is checkable in C as-is, because "max - b" cannot underflow. If the condition evaluates to true, then "a + b" won't overflow. (3) In case OP is "*": a * b <= max we need to see if "b" is zero, at first. If "b" is zero, the multiplication is safe. If "b" is nonzero, then we can divide both sides by it: a <= max / b This condition is checkable in C as-is, because "max / b" cannot wrap, or involve a division by zero. However, unlike in the addition/subtraction case (2), the *equivalence* of this "rearrangement as a division" with the original a * b <= max is not immediately clear, given that the "/" operator in C (between integer operands) is not an exact division, but division with a remainder. As the standard says, "the result ... is the algebraic quotient with any fractional part discarded". The usual way to prove "equivalence" between two logical formulae is to prove "implication" in both directions. That is, assuming "b" is nonzero, let's prove both of the following implications between C-language expressions: (a <= max / b) IMPLIES (a * b <= max) [1] (a * b <= max) IMPLIES (a <= max / b) [2] (5) At first, let's express what the max / b == q [3] expression in C means, expressed *mathematically*. That way, we can investigate [1] and [2] in pure math. The C expression [3] means, mathematically speaking: max = q * b + r [4] where "q" is a nonnegative integer, and "r" is a nonnegative integer strictly smaller than "b". And, this decomposition is unique, given any pair of (max, b). (And we posited earlier that "b" is nonzero, of course.) (6) Now, going back to [1] and [2]: - wherever we have a (max / b) C-language expression, substitute "q" (from [3]; - wherever we have a standalone "max" expression, substitute "q * b + r" (from [4]). We get the following *mathematical* (not C!) expressions: (a <= q) IMPLIES (a * b <= q * b + r) [5] (a * b <= q * b + r) IMPLIES (a <= q) [6] (7) Implication [5] is trivially true, as we multiply both sides of the premise inequality (a <= q) with the positive integer "b" at first: a * b <= q * b and then add the nonnegative integer "r" to the right hand side only: a * b <= q * b + r (8) Implication [6] is slightly more tricky. First, let's divide both sides of the premise inequality (a * b <= q * b + r) with the positive integer "b": a * b q * b + r ----- <= --------- b b That simplifies to: a <= q + (r/b) [7] Here's where we consider the restriction on "r" from [4]: "r" is a nonnegative integer *strictly smaller* than "b". That gives us, for the (r/b) addend in separation: 0 <= (r / b) < 1 Which means that we can tack another (strict) inequality to the right side of [7]: a <= q + (r/b) < q + 1 [8] ^^^^^^^ (Because, adding (r/b) to "q" produces a smaller number than adding 1 to "q" does.) Considering the two far sides of [8] together, we get a < q + 1 Then, as both "a" and "q" are integers, this is equivalent to a <= q which is what we wanted to reach in [6]. (9) And so the C-language functions are just: bool add_size_t_overflow (size_t a, size_t b, size_t *result) { if (a <= SIZE_MAX - b) { *result = a + b; return false; } return true; } bool mul_size_t_overflow (size_t a, size_t b, size_t *result) { if (b == 0 || a <= SIZE_MAX / b) { *result = a * b; return false; } return true; } (10) The (unsigned) type-generic macros could look something like this. First, we'd need to check if the types of "a", "b" and "result" were (i) integers, (ii) unsigned, and (iii) had identical range. If not, we should get a compilation failure (static assert, of sorts). Let's assume at least that none of our integer types have padding bits, and that signed integer types use two's complement. (Although the signed representation may be irrelevant in the discussion below.) I don't consider bit-fields at all because they are pure evil anyway. :) (i) The "integer" check for any arithmetic type can be done by dividing ((type)1) with 2. Every floating point type can represent 1/2 exactly (so the result compares uneqal to zero), and for every integer type, the result is zero. (ii) The "unsigned" check can be done by seeing if ((type)-1) is positive. This works even for _Bool, plus -1 is safely expressible by any signed integer type (IOW even if the check fails, it is safe to evaluate). (iii) "identical range" can be expressed by ((type1)-1 == (type2)-1), with the additional -- perhaps redundant -- restriction (sizeof a == sizeof b). This relies on our assumption about integer representation (no padding bits). The static assert can be done by intentionally violating a C constraint (which in turn requires a diagnostic to be emitted), dependent on an integer constant expression that combines (i), (ii) and (iii). A common trick is to create an array typedef, where the size of the array is -1 (if the condition we want to enforce fails) vs. +1 (otherwise). (11) Then we could do this, for example: #include <stdbool.h> #include <string.h> /* This macro does not evaluate any one of its arguments, unless the argument * has variably modified type. */ #define HAVE_SAME_UINT_TYPES(a, b) \ ((typeof (a))1 / 2 == 0 && \ (typeof (b))1 / 2 == 0 && \ (typeof (a))-1 > 0 && \ (typeof (b))-1 > 0 && \ (typeof (a))-1 == (typeof (b))-1 && \ sizeof a == sizeof b) /* This macro evaluates each of "a", "b" and "r" exactly once, assuming neither * has variably modified type. */ #define ADD_UINT_OVERFLOW(a, b, r) \ ({ \ typedef char static_assert_1[-1 + 2 * HAVE_SAME_UINT_TYPES(a, b)]; \ typedef char static_assert_2[-1 + 2 * HAVE_SAME_UINT_TYPES(a, *r)]; \ typeof (a) a2 = a, b2 = b, max = -1, result; \ void *r2 = r; \ bool overflow = true; \ \ if (a2 <= max - b2) { \ result = a2 + b2; \ memcpy(r2, &result, sizeof result); \ overflow = false; \ } \ overflow; \ }) Note that using this would require a bit more pedantry than usual at the *call sites*; for example, adding constants "1" and "2u" would not work. TBH, I think the macros I created above are hideous. I like call sites to be explicit about types, and so I'd prefer the much simpler; type-specific add_size_t_overflow() and mul_size_t_overflow() functions. (Beyond reverting b31859402d14, I'd even replace the type-specific ADD_UINT64_T_OVERFLOW, MUL_UINT64_T_OVERFLOW, ADD_SIZE_T_OVERFLOW, MUL_SIZE_T_OVERFLOW function-like macros with actual functions. Just let the compiler inline whatever it likes.) Comments? Thanks Laszlo

On Thu, Nov 18, 2021 at 01:39:39PM +0100, Laszlo Ersek wrote:

(... long email)

And quite insightful. I'm trimming to portions where I'm replying...

- signed integers are a huge complication, while at the same time, their use (or usefulness) is extremely limited for dealing with memory allocations, sizes, indexing, file offsets, and so on.

Actually, the emacs project has been moving towards explicitly using signed types for memory sizes. Why? Because compilers have modes where they can diagnose signed overflows, but the C standard requires unsigned types to perform module 2^N arithmetic without warning. So using a signed type (gnulib calls it idx_t) that is otherwise the same width as size_t makes it possible for the compiler to help diagnose non-portable code. But yse, writing code that avoids overflow using signed types is a much trickier proposition.

I wouldn't try to do this in a type-transparent manner. I'd first revert commit b31859402d14 ("common/include/checked-overflow.h: Simplify", 2021-11-11), and then add separate functions for uint64_t and size_t.

Writing functions would allow a and b to undergo integer promotions, but the return type pointer forces an explicit choice of the result type. Unless we tell the compiler to warn on narrowing of integers to a function call, there is a risk that on 32-bit machines, someone passing a 64-bit off_t value to a 32-bit size_t overflow checker will undergo inadvertent overflow prior to the function getting a chance to perform its checks. One of the benefits of the gcc/clang builtin is that they are type-agnostic, even if I want to perform off_t*int and store the result in size_t, the builtin will correctly tell me if the infinite-precision result would overflow the destination, without me having to figure out whether promotion or implicit narrowing affected things. Using a function loses some of that benefit when compared to a fully-generic macro. However, writing a fully-generic macro that correctly handles mismatched operand types is indeed harder.

If we don't want to be 100% standard C, just make it work for gcc-4.2, then a type-generic(-ish) solution could exist using "typeof" and (perhaps) statement expressions. (Both are GCC extensions that exist in 4.2: - <https://gcc.gnu.org/onlinedocs/gcc-4.2.4/gcc/Statement-Exprs.html> - <https://gcc.gnu.org/onlinedocs/gcc-4.2.4/gcc/Typeof.html>.)

Indeed, that's also an intermediate option - we require gcc (even on RHEL 7) for our automatic cleanup usage, so relying on the features present in the oldest supported gcc is acceptable, rather than having to strive for strict C compliance.

To go into some details:

(1) The idea is that we want the following C expression to evaluate to true, without OP overflowing first:

a OP b <= max

where "a", "b", "max" are all of the same unsigned integer type, and OP is either "+" or "*".

Your analysis proceeds with all of "a", "b", and "max" of the same unsigned types. I understand why you skipped signed types (the analysis is harder); but it it worth supporting cases where "a" and "b" are possibly different widths? The gnulib file intprops.h does this, but of course, due to the incompatible licensing, me repeating how it does it here is not in our best interest (however, I will point out that it results in a MUCH larger preprocessor expansion than your simpler solution that enforces same size inputs).

(9) And so the C-language functions are just:

bool add_size_t_overflow (size_t a, size_t b, size_t *result) { if (a <= SIZE_MAX - b) { *result = a + b; return false; } return true;

The compiler builtins ALSO assign the low-order bits of the infinite-precision operation into *result on overflow, behaving more like: *result = a + b; return ! (a <= SIZE_MAX - b); for unsigned types (where C has guaranteed modulo semantics), and a much more complicated solution with signed types (basically, perform the operation in the unsigned counterparts, and then cast those low-order bits back to signed at the end).

}

bool mul_size_t_overflow (size_t a, size_t b, size_t *result) { if (b == 0 || a <= SIZE_MAX / b) { *result = a * b; return false; } return true; }

(10) The (unsigned) type-generic macros could look something like this. First, we'd need to check if the types of "a", "b" and "result" were (i) integers, (ii) unsigned, and (iii) had identical range.

Check (i) is a nice safety valve. Check (ii) is essential to avoid the nastiness that would be required for correct results with signed types, but with a larger macro, we could indeed support it (gnulib demonstrates that). But I'm inclined to insist on (ii) until we encounter a case where we really care about signed overflow; insisting that we use unsigned sizes (contrary to emacs' choice of using a signed idx_t type for compiler assistance) is easier to maintain, even if it reduces the places where the compiler can help us. Check (iii) is the one I question as being strictly necessary; if we can correctly predict the size of the result after arithmetic promotion, we can determine the maximum value that we must fit within even if the inputs underwent a size change. ...

I don't consider bit-fields at all because they are pure evil anyway. :)

Too true! ...

(11) Then we could do this, for example:

#include <stdbool.h> #include <string.h>

/* This macro does not evaluate any one of its arguments, unless the argument * has variably modified type. */ #define HAVE_SAME_UINT_TYPES(a, b) \ ((typeof (a))1 / 2 == 0 && \ (typeof (b))1 / 2 == 0 && \ (typeof (a))-1 > 0 && \ (typeof (b))-1 > 0 && \ (typeof (a))-1 == (typeof (b))-1 && \ sizeof a == sizeof b)

Yes, this is relying on gcc's typeof extension, but we said above that should be okay.

/* This macro evaluates each of "a", "b" and "r" exactly once, assuming neither * has variably modified type. */ #define ADD_UINT_OVERFLOW(a, b, r) \ ({ \ typedef char static_assert_1[-1 + 2 * HAVE_SAME_UINT_TYPES(a, b)]; \ typedef char static_assert_2[-1 + 2 * HAVE_SAME_UINT_TYPES(a, *r)]; \

We could perhaps relax this to: typedef char static_assert[-1 + 2 * HAVE_SAME_UINT_TYPES((a)+(b), *r)]; \ if we merely care about the promoted result type having the same type as where we are storing the result, and if we also assert that sizeof(*r)>=sizeof(int) (for result types smaller than int, the logic is a lot messier, because of arithmetic promotion).

typeof (a) a2 = a, b2 = b, max = -1, result; \ void *r2 = r; \ bool overflow = true; \ \ if (a2 <= max - b2) { \ result = a2 + b2; \ memcpy(r2, &result, sizeof result); \ overflow = false; \ } \ overflow; \ })

Note that using this would require a bit more pedantry than usual at the *call sites*; for example, adding constants "1" and "2u" would not work.

Per the above, constant "1" would never work (it is a signed constant). That's why figuring out a macro that works with arbitrary type inputs, so long as their integer promotion doesn't overflow (which is what the compiler builtins do) is nicer, but indeed harder. Using a function instead of a macro gets you the implicit promotion (you can pass 1 to a function taking size_t, whereas the macro computing typeof(1) will see a signed type).

TBH, I think the macros I created above are hideous. I like call sites to be explicit about types, and so I'd prefer the much simpler; type-specific add_size_t_overflow() and mul_size_t_overflow() functions.

Your macros were not too hideous, but only because they made limiting assumptions like forcing unsigned types and not caring about arithmetic promotion. The gnulib macros are more generic, but even more hideous.

(Beyond reverting b31859402d14, I'd even replace the type-specific ADD_UINT64_T_OVERFLOW, MUL_UINT64_T_OVERFLOW, ADD_SIZE_T_OVERFLOW, MUL_SIZE_T_OVERFLOW function-like macros with actual functions. Just let the compiler inline whatever it likes.)

Comments?

As long as we have good unit tests for the particular use cases we care about detecting, I can live with either a generic macro or with type-specific functional interfaces. I'm a bit worried that using functions may fail to catch overflow on the implicit type conversions of arguments passed to the function; maybe it's possible to do a hybrid, by writing a function that checks for type equality but then calls a function for the actual overflow check, so that we are ensured that callers use the correct types, but have the ease of reading the overflow check in a function that doesn't require even more macro expansions to see what it is doing. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Thu, Nov 18, 2021 at 06:24:09PM +0100, Laszlo Ersek wrote:

...

Your analysis proceeds with all of "a", "b", and "max" of the same unsigned types. I understand why you skipped signed types (the analysis is harder); but it it worth supporting cases where "a" and "b" are possibly different widths? The gnulib file intprops.h does this, but of course, due to the incompatible licensing, me repeating how it does it here is not in our best interest (however, I will point out that it results in a MUCH larger preprocessor expansion than your simpler solution that enforces same size inputs).

I imagine we could take each input (i.e., "a", "b") as uintmax_t, perform the addition / multiplication in uintmax_t (with the safeguards I proposed earlier adapted to UINTMAX_MAX), and then explicitly compare the result against the maximum of the desired result type, such as UINT64_MAX, SIZE_MAX, and so on.

I didn't think of this because I'm acutely aware of types, promotions etc when I write C code, so "these two types must be the same" is not something I'd be bothered by.

It's also a tradeoff on how generic we happen to be, vs. whether all our callers actually do use the same size. If the code is easier to maintain by insisting on callers matching certain patterns, that's okay as long as we can easily detect callers that fail those patterns. But if the cost of detecting out-of-spec callers is worse than just biting the bullet and writing the code to support mixed types, then being fully generic wins out.

...

...

(9) And so the C-language functions are just:

bool add_size_t_overflow (size_t a, size_t b, size_t *result) { if (a <= SIZE_MAX - b) { *result = a + b; return false; } return true;

The compiler builtins ALSO assign the low-order bits of the infinite-precision operation into *result on overflow, behaving more like:

*result = a + b; return ! (a <= SIZE_MAX - b);

Ah, interesting. I didn't notice that. I figured "output is indeterminate on error" is generally acceptable. Is there a particular use for the low-order bits, on overflow?

I suspect there may be some backwards compatibility at play, and most hardware is already able to give us that for free. Even though you're right that most sane users of these macros won't use the result on overflow, I'm more worried about the subtle bugs that would occur on less-tested platforms if we don't actually provide a deterministic value (particularly if most of our testing occurs on platforms where the compiler builtins do have deterministic behavior). ...

Right, I didn't consider types with lesser integer conversion rank than that of "unsigned int" -- I don't think we'd use "short" for memory allocation, sizes, indexing, file offsets etc.

While it sounds impractical, I do have to wonder if anyone has ever tried to detect running out of TCP ports (a "short") by using overflow checking.

I wouldn't want to be more gnulib than gnulib :) (Not that I looked at it -- I intentionally didn't.)

And that's a good thing - since I'm tainted by knowing what gnulib has done, I'm trying to make sure I keep it possible for you to come up with a clean-room implementation if we want it, even though I shouldn't write it myself. ...

...

...

Comments?

As long as we have good unit tests for the particular use cases we care about detecting, I can live with either a generic macro or with type-specific functional interfaces. I'm a bit worried that using functions may fail to catch overflow on the implicit type conversions of arguments passed to the function; maybe it's possible to do a hybrid, by writing a function that checks for type equality but then calls a function for the actual overflow check, so that we are ensured that callers use the correct types, but have the ease of reading the overflow check in a function that doesn't require even more macro expansions to see what it is doing.

Let me try this (later, only a sketch now):

- use macros for checking that the input and output types are (ii) unsigned (i) integer types, ignore their (iii) particular ranges

- convert the inputs to uintmax_t and perform the operation in uintmax_t unconditionally, only *remembering* -- using the same in-advance checks -- if UINTMAX_MAX is overflowed,

Perhaps you can do this with a function signature like: bool check_op_overflow (uint64_t a, uint64_t b, uint64_t max, uint64_t *r)

- deduce the actual result limit from ((typeof (*r))-1),

- assign the low-order bits in any case -- if either the uintmax_t operation overflows or the actual result limit is exceeded, it all happens in unsigned, so it's well-defined (modular reduction). Return the overflow status correctly.

and then utilize it something like: #define CHECK_OP_OVERFLOW(a, b, r) \ ({ \ bool overflow; uint64_t tmp; \ CHECK_UNSIGNED_INT (a); CHECK_UNSIGNED_INT (b); \ overflow = check_op_overflow (a, b, (typeof (*(r)))-1, &tmp); \ *(r) = tmp; \ overflow; \ }) and maybe even have CHECK_UNSIGNED_INT() permit signed values >= 0, rather than insisting that the underlying type be unsigned. At any rate, the approach sounds good to me. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Fri, Nov 19, 2021 at 03:35:48PM +0100, Laszlo Ersek wrote:

(1) What is the usual style in nbdkit to highlight parameters of functions and function-like macros in documentation (comments)? Here I used parentheses. Normally I use quotes "".

We don't have one, so quotes are fine.

(2) In the actual functions, we return "true" for "overflow". My original conditions expressed "no overflow". In this source code, I *open-coded* the negations of my original conditions, using De Morgan's laws. That's because I dislike expressions with an outermost "!" operator -- whenever I face such an expression, I always work the outermost negation *into* the formula, as deeply as I can.

However, I realize the resultant conditions may not be easy to read this way. Here's an alternative for the body of check_add_overflow(), based on the original formulation:

register bool in_range;

*r = a + b; in_range = a <= UINTMAX_MAX - b && *r <= max; return !in_range;

Seems reasonable.

(3) In the overflow conditions, I used parentheses as sparingly as possible; only when needed. I could also use as many as possible.

- Compare, for the addition:

return ((a > UINTMAX_MAX - b) || (*r > max));

or

in_range = ((a <= UINTMAX_MAX - b) && (*r <= max)); return !in_range;

- Compare, for the multiplication:

return ((b > 0) && ((a > UINTMAX_MAX / b) || (*r > max)));

or

in_range = ((b == 0) || ((a <= UINTMAX_MAX / b) && (*r <= max))); return !in_range;

What's the preferred form?

Personally I use fewest parens, but indicate precedence by dropping whitespace, but I guess whatever works. However you definitely don't need the parens around the entire return value, C doesn't require that. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

On 11/18/21 16:27, Eric Blake wrote:

On Thu, Nov 18, 2021 at 01:39:39PM +0100, Laszlo Ersek wrote:

...

(10) The (unsigned) type-generic macros could look something like this. First, we'd need to check if the types of "a", "b" and "result" were (i) integers, (ii) unsigned, and (iii) had identical range.

Check (i) is a nice safety valve.

Meanwhile I've realized that (typeof (a))-1 > 0 covers (i) and (ii) both, because all floating point types can represent -1 (and exactly at that). So there is no need for (typeof (a))1 / 2 == 0 just for (i). Thanks Laszlo

On 11/22/21 23:31, Richard W.M. Jones wrote:

(Catching up ...)

Welcome back! :)

The maths looked reasonable.

Thanks!

Did you have a version of the patch for review?

No, not yet. Wanted to clear the style questions on the new code (the additions) at first. Next, I'll have to work those additions (the fallback code) into your original patch -- I'll steal the way you check for the necessity of the fallback etc. (Meanwhile I've worked on RHBZ#1931821, with the realization that the "dosfstools" change was inexcusable, and either way, we need "parted" to learn dealing with the bogus partition table.)

My only other thought is that a simple set of tests could be good. However it's not worth having tests that only test if __builtin* functions are correct (hopefully GCC is already testing that). So tests would have to check the fallback macros are correct, even if they are not used on the current platform.

So: the fallbacks need to be available (= built) in the source code unconditionally, so they can be directly called by the test suite. The actual "falling back" to them must be separate. Is that what you mean? Thanks Laszlo

On Tue, Nov 23, 2021 at 04:19:08PM +0100, Laszlo Ersek wrote:

On 11/23/21 11:44, Laszlo Ersek wrote:

...

So: the fallbacks need to be available (= built) in the source code unconditionally, so they can be directly called by the test suite. The actual "falling back" to them must be separate. Is that what you mean?

... should this go into common/replacements, or common/utils? The former seems like a better fit.

common/replacements is currently used to replace functions which are missing on Windows. Maybe this could be extended for RHEL 7? Probably easier to put it in common/utils/checked-overflow.h TBH (see my original version of the patch which did that and used a configure check to select which version is used).

On the other hand, libnbd (assuming we'll want to port the same to libnbd) does not have common/replacements at all...

Indeed, and libnbd is never likely to get a Windows port. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

On Tue, Nov 23, 2021 at 03:25:22PM +0000, Richard W.M. Jones wrote:

On Tue, Nov 23, 2021 at 11:44:06AM +0100, Laszlo Ersek wrote:

...

On 11/22/21 23:31, Richard W.M. Jones wrote:

...

My only other thought is that a simple set of tests could be good. However it's not worth having tests that only test if __builtin* functions are correct (hopefully GCC is already testing that). So tests would have to check the fallback macros are correct, even if they are not used on the current platform.

So: the fallbacks need to be available (= built) in the source code unconditionally, so they can be directly called by the test suite. The actual "falling back" to them must be separate. Is that what you mean?

I just meant that if we have a test suite which tests the end result (macros like "ADD_OVERFLOW"), because we will only do CI on modern systems, it will only ever test that __builtin_add_overflow works. That's pointless because presumably GCC already tests that, and the fallback code would never be tested. So the tests would need to somehow check that the fallback path works.

Something along the lines of: #if (...gcc or clang new enough...) && !defined(NBDKIT_FALLBACK) # define ADD_OVERFLOW(a, b, r) __builtin_add_overflow((a), (b), (r)) #else # define ADD_OVERFLOW(a, b, r) ...fallback... #endif so that the testsuite can then #define NBDKIT_FALLBACK for testing the fallback code even on new enough compilers. I'm not sure how best to namespace the witness macro for forcing a fallback if the code is copied between nbdkit and libnbd. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On 11/17/21 18:57, Richard W.M. Jones wrote:

On Mon, Nov 15, 2021 at 02:00:25PM +0100, Laszlo Ersek wrote:

...

On 11/15/21 13:14, Richard W.M. Jones wrote:

(aren't you on PTO?)

Eh hem ..

...

...

-# This is somewhat different from the other tests because we have -# to build an actual plugin here.

(1) I think this comment removal belongs to a separate patch. (Not a "hard requirement", just saying.)

Done, with clearer explanation: https://gitlab.com/nbdkit/nbdkit/-/commit/6b821fdc4ced4fc0af3fe2480d3ca43cb6...

...

...

+ char *args[] = { + "nbdkit", "-s", "--exit-with-parent", + "./test-ocaml-errorcodes-plugin.so", NULL + };

(3) Style (well, at least my personal preference, not sure about nbdkit standards): this should be

static const char * const args[] = ...

or at least (cue <https://libguestfs.org/nbd_connect_command.3.html>)

static char *args[] = ...

and in either case it should go to the top of the block (not mixing declarations with code).

Feel free to ignore of course if the pattern is already wide-spread in nbdkit.

Thie pattern has been copied around over about a dozen tests. How about attached instead? Compound literals are a fun, little know feature!

Looks fine to me, thanks! Laszlo

...

...

+ assert (nbd_pread (nbd, buf, 512, 1*512, 0) == -1); + printf ("%s %d\n", nbd_get_error (), nbd_get_errno ()); + fflush (stdout); + assert (nbd_get_errno () == EPERM); + + assert (nbd_pread (nbd, buf, 512, 2*512, 0) == -1); + printf ("%s %d\n", nbd_get_error (), nbd_get_errno ()); + fflush (stdout); + assert (nbd_get_errno () == EIO); + + assert (nbd_pread (nbd, buf, 512, 3*512, 0) == -1); + printf ("%s %d\n", nbd_get_error (), nbd_get_errno ()); + fflush (stdout); + assert (nbd_get_errno () == ENOMEM); + + assert (nbd_pread (nbd, buf, 512, 4*512, 0) == -1); + printf ("%s %d\n", nbd_get_error (), nbd_get_errno ()); + fflush (stdout); + assert (nbd_get_errno () == ESHUTDOWN); + + assert (nbd_pread (nbd, buf, 512, 5*512, 0) == -1); + printf ("%s %d\n", nbd_get_error (), nbd_get_errno ()); + fflush (stdout); + assert (nbd_get_errno () == EINVAL);

(4) This shouldn't be difficult to turn into a table of { sector, errno } pairs, and a loop. Not necessarily for the space savings, but for ease of reading. (I've found it hard to discern the only two differences between each repetition of the block.)

(5) The fflush(stdout) looks weird; if that really matters (e.g. for following the log when it is written to a regular file), then we might want to consider setvbuf() instead. Doesn't matter much if you implement (4), because in that case, the loop body will contain only one fflush().

I fixed this all up in v2 which I'll post shortly.

Rich.