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

Cleanup fd shuffle #4877

Merged

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Oct 22, 2021

Builds on #4872

Changelog-None

@rustyrussell rustyrussell requested a review from ZmnSCPxj October 22, 2021 00:14
lightningd/subd.c Show resolved Hide resolved
lightningd/subd.c Outdated Show resolved Hide resolved
lightningd/subd.c Show resolved Hide resolved
Comment on lines 24 to 25
size_t i;
for (i = 0; i < len; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't catch we need to use this old syntax for the for loop, why not just?

Suggested change
size_t i;
for (i = 0; i < len; i++) {
for (size_t i = 0; i < len; i++) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the ccan library, which seeks to be more portable. Declarations in for are C99, but the ccan library might very well be compiled with an older C standard.

C-lightning is C99 (actually it is GNU C99, i.e. C99 with some GNU-specific extensions) but the ccan sub-library is technically a separate project that is upstream of C-lightning, so we just copy the ccan code. And as mentioned, ccan could be compiled in an older C standard, so the ccan modules have to use older standards.

@@ -115,7 +116,8 @@ pid_t pipecmdarr(int *fd_tochild, int *fd_fromchild, int *fd_errfromchild,
goto fail;

if (childpid == 0) {
for (int i = 0; i < num_child_close; i++)
int i;
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo Oct 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh, sure that I'm missing something related to C? Here we could avoid the double declaration of i, but not sure that it is that the motivation

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I have only some small comments

It's both complex and flawed, as ZmnSCPxj points out.  Make a generic
fd ordering routine, and use it.

Plus, test it!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell
Copy link
Contributor Author

Fixed from @ZmnSCPxj feedback and rebased...

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack a7d35c4

Copy link
Contributor

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a7d35c4

Just needs a ChangeLog-None.

@rustyrussell rustyrussell merged commit 78ebdde into ElementsProject:master Nov 9, 2021
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.

3 participants