Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove calls to error() from error.h. #787

Merged
merged 3 commits into from
Feb 13, 2023
Merged

Remove calls to error() from error.h. #787

merged 3 commits into from
Feb 13, 2023

Conversation

imwints
Copy link

@imwints imwints commented Feb 13, 2023

On musl systems, liburing cannot build examples and tests due to it's usage of error.h. Replace all calls to error() with fprintf(stderr, ...) and exit(1).

Closes: #786

Signed-off-by: Steffen Winter steffen.winter@proton.me

examples/send-zerocopy.c Outdated Show resolved Hide resolved
@axboe
Copy link
Owner

axboe commented Feb 13, 2023

I'd add a t_error() in the test helpers as a first patch, then a second patch that converts any error() usage to that instead. Will make it much easier to verify that this is a 1-1 substitution and reduces the boiler plate stuff.

@imwints
Copy link
Author

imwints commented Feb 13, 2023

I was not quite sure where to put it. Should t_error have the same function definition as error?

@ammarfaizi2
Copy link
Contributor

I was not quite sure where to put it.

test/helpers.c (definition) and test/helpers.h (declaration)

@axboe
Copy link
Owner

axboe commented Feb 13, 2023

test/helpers.h and test/helpers.c contains code like this to avoid every test needing to re-invent the wheel. And yes, just make it identical to error(), just use va_list and friends for this.

@ammarfaizi2
Copy link
Contributor

ammarfaizi2 commented Feb 13, 2023

I was not quite sure where to put it.

test/helpers.c (definition) and test/helpers.h (declaration)

Oh, wait... you can't use that helper from examples/, so I guess we have
to duplicate it in examples/ too.

test/helpers.h Outdated Show resolved Hide resolved
test/helpers.c Outdated Show resolved Hide resolved
test/helpers.c Outdated Show resolved Hide resolved
test/poll.c Outdated Show resolved Hide resolved
test/single-issuer.c Outdated Show resolved Hide resolved
test/helpers.c Show resolved Hide resolved
test/helpers.c Outdated Show resolved Hide resolved
test/helpers.c Outdated Show resolved Hide resolved
@axboe
Copy link
Owner

axboe commented Feb 13, 2023

Looks good to me know, just a few minor nits that would be nice to address and this can go in.

@ammarfaizi2
Copy link
Contributor

Add the background story to the commit message, please.
Also, link to the GitHub issue. Something like "Fixes" or "Closes"?

@ammarfaizi2
Copy link
Contributor

On musl systems, liburing cannot build examples and tests due to it's usage of error.h. Replace all calls to error() with fprintf(stderr, ...) and exit(1).

Closes: #786

This should go to the commit message.

On musl systems, liburing cannot build examples and tests due to
it's usage of error.h. t_error calls fprintf(stderr, ...) and
exits.

Closes: #786

Signed-off-by: Steffen Winter <steffen.winter@proton.me>
@imwints
Copy link
Author

imwints commented Feb 13, 2023

What about examples/send-zerocopy.c? It has a lot of references to error(), is it okay to just slap an implementation of t_error at the top?

@axboe
Copy link
Owner

axboe commented Feb 13, 2023

Yeah, just put it in that example file too. With that done and the commit messages update as requested, I think this is good to go.

@ammarfaizi2
Copy link
Contributor

What about examples/send-zerocopy.c? It has a lot of references to error(), is it okay to just slap an implementation of t_error at the top?

Yeah, I think it's the sanest way for now. Integrating test/helpers.c to examples/ doesn't make much sense.

@axboe
Copy link
Owner

axboe commented Feb 13, 2023

Don't have identical git commit titles. Prefix one with test: and the other with examples: or something like that.

On musl systems, liburing cannot build examples and tests due to
it's usage of error.h. Replacing calls to error() with t_error().

Closes: #786

Signed-off-by: Steffen Winter <steffen.winter@proton.me>
@axboe
Copy link
Owner

axboe commented Feb 13, 2023

Please fold the last two commits, we should not have a single PR introducing a problem and then a fixup for that afterwards.

@ammarfaizi2
Copy link
Contributor

t_error in examples/send-zerocopy.c needs to be static.

send-zerocopy.c:63:6: error: no previous prototype for function 't_error' [-Werror,-Wmissing-prototypes]
void t_error(int status, int errnum, const char *format, ...)
     ^
send-zerocopy.c:63:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void t_error(int status, int errnum, const char *format, ...)
^
static 
1 error generated.
make[1]: *** [Makefile:36: send-zerocopy] Error 1

On musl systems, liburing cannot build examples and tests due to
it's usage of error.h. t_error copied from test/helpers.c.
Replacing calls to error() with t_error().

Closes: #786

Signed-off-by: Steffen Winter <steffen.winter@proton.me>
@ammarfaizi2
Copy link
Contributor

Looks good.

@axboe axboe merged commit b9231b5 into axboe:master Feb 13, 2023
@imwints imwints deleted the remove-non-portable-error branch February 13, 2023 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

liburing-2.3 build failure on musl (uses non-portable error.h)
3 participants