Skip to content

Conversation

rootkea
Copy link
Contributor

@rootkea rootkea commented Sep 10, 2021

No description provided.

@masatake masatake self-assigned this Sep 11, 2021
@masatake
Copy link
Member

Thank you for taking your time for reporting this.
Your changes are mostly correct.

I inspected the failure of the test.

The functions taking tagFile *file expects file->err == 0.
The functions sets file->err = TagErrnoInvalidArgument if file->err != 0.

This is expected behavior in tests/test-api-tagsSetSortType.c.
Your code changes the behavior.

To keep our API, changes are needed on your changes like:

diff --git a/readtags.c b/readtags.c
index 942a2c73..0173c841 100644
--- a/readtags.c
+++ b/readtags.c
@@ -1038,10 +1038,10 @@ static tagResult findSequentialFull (tagFile *const file,
 									 int (* isAcceptable) (tagFile *const, void *),
 									 void *data)
 {
-	if (file == NULL || file->err)
+	if (file == NULL)
 		return TagFailure;
 
-	if (!file->initialized)
+	if (!file->initialized || file->err)
 	{
 		file->err = TagErrnoInvalidArgument;
 		return TagFailure;

Could you update your change?

You can run the test locally with "make check".

@rootkea
Copy link
Contributor Author

rootkea commented Sep 11, 2021

The functions sets file->err = TagErrnoInvalidArgument if file->err != 0.

Then aren't we overwriting the err? We will be printing TagErrnoInvalidArgument instead of the actual original error occurred.

@masatake
Copy link
Member

We will be printing TagErrnoInvalidArgument instead of the actual original error occurred.

Overwriting is the expected behavior.

@rootkea rootkea marked this pull request as draft September 11, 2021 09:57
@masatake
Copy link
Member

masatake commented Sep 11, 2021

Do you have a plan to add more commits to this pull request?
If not, press the "Ready for review" button.

I will merge this. I will add the following entry to NEWS.md file:

- fix potential crashes trigged when passing NULL as `file` parameter
  to the API functions. Provided by Avinash Sonawane.

@rootkea rootkea marked this pull request as ready for review September 11, 2021 10:30
@rootkea
Copy link
Contributor Author

rootkea commented Sep 11, 2021

Ready for review!

Please let me know if something needs to be changed/removed. :)

@rootkea
Copy link
Contributor Author

rootkea commented Sep 11, 2021

Provided by Avinash Sonawane.

BTW, I'll appreciate if you could make it as Provided by rootkea. :)

@masatake
Copy link
Member

For making understanding the changes from git log --oneline easier, could you add prefixes to the header of each commit?

buildsys: set executable (+x) permission on autogen.sh
tests: remove redundant argument
tests:  remove redundant code 
tests:  use snprintf() instead of sprintf()

This request for adding prefixes is applicable to your pull request sent to ctags repository, too.

BTW, I'll appreciate if you could make it as Provided by rootkea. :)

I see.

@rootkea
Copy link
Contributor Author

rootkea commented Sep 15, 2021

For making understanding the changes from git log --oneline easier, could you add prefixes to the header of each commit?

Done!

@masatake masatake merged commit 357a240 into universal-ctags:master Sep 15, 2021
@masatake
Copy link
Member

Thank you!

@masatake
Copy link
Member

Updating NEWS.md is done.

@masatake
Copy link
Member

The next step of mine is pulling the change into the ctags source tree.
Updating universal-ctags/python-ctags3 is another task.

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.

2 participants