-
Notifications
You must be signed in to change notification settings - Fork 103
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
Fix musl build #206
Fix musl build #206
Conversation
I have tested that it builds on fedora but not tested if it actually works as intended. I would appreciate help doing so. Thanks! EDIT: I have now build tested it on GNU system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your commit messages contain content that's a bit... off. I can fix them on merge, but do need to call out what's up here.
"The *outfile passed to parse_input_files can only be either NULL or set to optarg. Neither should be free'ed." While it's true that optarg is not to be freed, it's perfectly fine to free(NULL). This is generally quite handy for simplifying cleanup code.
"on_exit(3) does not provide any real value since the arguments passed needs to be globals anyway." I don't see how you can claim this since the code you're proposing for atexit() is strictly more complex. It's even contradicted by the next sentence :)
Nice to meet someone who actually cares about commit messages 🙂 I'd be happy to fix the commit message myself so I learn how to better next time.
I can update the commit message unless you want the commit removed. (since we set optarg to NULL is should be be safe'ish)
Well. yes. not my best commit message. I initially thought that the only reason for on_exit to exist was to pass the arg pointer, at which point I found the use of on_exit meaningless in this case. I later discovered that status was not passed to atexit, and yeah... not might best commit message. Another option I was thinking of was to call @@ -587,24 +590,30 @@ sort_err:
outfd = open(outfile, flags, 0600);
if (outfd < 0) {
char *tmpoutfile = outfile;
- if (errno == EEXIST)
- outfile = NULL;
+ if (errno != EEXIST)
+ unlink(outfile);
err(1, "could not open \"%s\"", tmpoutfile);
}
rc = ftruncate(outfd, 0);
- if (rc < 0)
+ if (rc < 0) {
+ unlink(outfile);
err(1, "could not truncate output file \"%s\"", outfile);
+ }
void *output;
size_t size = 0;
rc = efi_secdb_realize(secdb, &output, &size);
- if (rc < 0)
+ if (rc < 0) {
+ unlink(outfile);
secdb_err(1, "could not realize signature list");
+ }
rc = write(outfd, output, size);
- if (rc < 0)
+ if (rc < 0) {
+ unlinke(outfile);
err(1, "could not write signature list");
+ }
close(outfd);
xfree(output); Thank you for useful feedback. |
I gave it a try to remove the atexit handler for unlinking outfile. I think it simplifies things a bit (but I still don't like the globals...).
What do you think? |
When I think about it, it would be better to do it all in a single commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear what your objection is to the globals: is there something concrete there? Because we're not opposed to them in the general sense.
Hello @ncopa It looks like you forgot about this pull request. I want to remind you about it |
Refactor code to use POSIX atexit(3) instead of the GNU specific on_exit(3). Resolves: rhboot#197 Resolves: rhboot#202 Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
The *outfile passed to parse_input_files can only be either set to optarg or be NULL. optarg should not be free'd and NULL does not need to. Since we no longer use on_exit to unlink outfile we also don't need to set *outfile to NULL. Fixes commit d917870 (efisecdb: add efisecdb) Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
Fix build with musl by using POSTIX atexit(3) instead of GNU specific on_exit(3).
Also fix potential double free in error handling while at it.
Fixes #197
Fixes #202