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

Fix CLN service startup failure by trimming trailing spaces in config parameters #7251

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maxrantil
Copy link
Collaborator

@maxrantil maxrantil commented Apr 22, 2024

fixes #7132

I would really appreciate some friendly feedback on this, as it is my first PR to the project and I might have misunderstood or missed important details.

I did find a similar function strip_trailing_whitespace in plugins/bcli.c, but from what I understand, having void trim_trailing_spaces(char *str); in ccan would still be beneficial, right?

Copy link
Collaborator

@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.

Thanks for this, looks good to me

I was wondering if it is possible move this trim function inside tal string https://github.com/rustyrussell/ccan/blob/master/ccan/tal/str/str.h

@maxrantil
Copy link
Collaborator Author

I was wondering if it is possible move this trim function inside tal string https://github.com/rustyrussell/ccan/blob/master/ccan/tal/str/str.h

Do you mean to also change the function to use tal for allocating memory for the lines then? I thought it was not necessary since they were already allocated, but I can of course fix that if it is desirable. @vincenzopalazzo

@vincenzopalazzo
Copy link
Collaborator

Do you mean to also change the function to use tal for allocating memory for the lines then?

No I mean to have like tal_strtrim or strtrim (not in tal), at the end of the day your code is implementing a subset of functionality to trim an origin string.

@maxrantil
Copy link
Collaborator Author

maxrantil commented Apr 24, 2024

Pardon me, but I don't fully understand what you mean. Could you elaborate on your suggestion? To my understanding, the tal functions you are linking to, @vincenzopalazzo , relate to string functions involving memory allocation. If I were to implement tal_strtrim(), from my experience, those implementations remove spaces from the beginning and end and allocate a new string. Is that what you are aiming for here? The current placement of the function trim_trailing_spaces() in ccan/ccan/str/str.c made sense to me because it manipulates an already allocated string.
Like I said at the beginning, I am new here and would appreciate a more thorough explanation if you have time for that.

Copy link
Collaborator

@endothermicdev endothermicdev left a comment

Choose a reason for hiding this comment

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

Nice PR! It's a neat solution to the problem. I agree with Vincenzo - so long as a ccan library is being modified, it would be nice to go ahead and trim the leading whitespace too and resize the buffer (which would make this a tal/str operation). It's not a significant difference in this case, but it could be more useful for the library. I'm okay with either approach.

@maxrantil
Copy link
Collaborator Author

Thanks for the comment @endothermicdev, I'll start doing a strtrim function in tal tomorrow if you both think that is more useful then 👍

@maxrantil
Copy link
Collaborator Author

@vincenzopalazzo & @endothermicdev , I've now changed to using tal_strtrim, is this what you had in mind?

@maxrantil maxrantil force-pushed the max/config-trim-spaces branch 4 times, most recently from e0cbfa2 to 3c829f0 Compare May 23, 2024 10:44
@maxrantil
Copy link
Collaborator Author

maxrantil commented May 23, 2024

@endothermicdev I am having some issues with tests on my local machine using tal_resize like this.
Is there anything in the way I am using it now, that you can see, is not working like it should? I get some out of memory errors from within the tal_resize() on my machine but the tests seems to run fine here in github-actions...
I am working on writing a test for it that I will push with the PR.

@maxrantil
Copy link
Collaborator Author

maxrantil commented May 23, 2024

The local test I have tried is to add a space after a config varible in the startup_regtest.sh file and then sourced it and start_ln 2.

@maxrantil maxrantil force-pushed the max/config-trim-spaces branch 3 times, most recently from 82b03ea to c8b062d Compare May 27, 2024 07:40
@maxrantil
Copy link
Collaborator Author

If someone could have a look at the pytest and tell me if something is wrong in it, that would be appreciated.

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.

Minor issue/question: Trailing spaces in Config file parameter make CLN fail to start
3 participants