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 errors made in merged PR #271 (commit b568955) #282

Merged
merged 1 commit into from Apr 7, 2023
Merged

Fix errors made in merged PR #271 (commit b568955) #282

merged 1 commit into from Apr 7, 2023

Conversation

ghost
Copy link

@ghost ghost commented Apr 6, 2023

This PR fixes errors made when creating PR #271. Please let me know if you would like to revert commit b568955 and have me prepare a fresh full PR or are happy just to apply this PR. I have compiled this fix in an Archlinux VM and all is well. Compiles fine in my fork, afp.conf is read correctly and shared volumes mount successfully.

@ghost ghost changed the title Fix errors made in previous commit Fix errors made in previous commit b568955 Apr 6, 2023
@ghost ghost changed the title Fix errors made in previous commit b568955 Fix errors made in merged PR #271 (commit b568955) Apr 6, 2023
@rdmark
Copy link
Member

rdmark commented Apr 6, 2023

As a general remark for future PRs, I'd love to see logic changes and stylistic changes in separate changesets. For one it's very hard to do code review effectively, and secondly each changeset should be crafted in a way that it facilitates future git blaming, as it were.

No need to go do anything with this PR though. I'm going to do some dynamic testing now so that I don't make the same mistake as last time. :)

@ghost
Copy link
Author

ghost commented Apr 6, 2023

Okeydoke. Will fix the two ifdefs and force push the changes to the iniparser branch tomorrow AM

@rdmark
Copy link
Member

rdmark commented Apr 6, 2023

Sounds good. Also, can you please make the commit message a bit more descriptive? See also: commit message guidelines on the wiki: https://github.com/Netatalk/netatalk/wiki/Developer-Notes#user-content-Commit_messages

@rdmark
Copy link
Member

rdmark commented Apr 6, 2023

This code produces the following warnings in my Debian Bullseye / arm / gcc10 environment.

make[3]: Entering directory '/home/dmark/dev/netatalk/libatalk/iniparser'
  CC       dictionary.lo
  CC       iniparser.lo
dictionary.c: In function ‘makekey’:
dictionary.c:65:5: warning: implicit declaration of function ‘strlcpy’; did you mean ‘strncpy’? [-Wimplicit-function-declaration]
   65 |     strlcpy(buf, section, MAXKEYSIZE);
      |     ^~~~~~~
      |     strncpy
dictionary.c:67:9: warning: implicit declaration of function ‘strlcat’; did you mean ‘strncat’? [-Wimplicit-function-declaration]
   67 |         strlcat(buf, ":", MAXKEYSIZE);
      |         ^~~~~~~
      |         strncat
  CCLD     libiniparser.la
make[3]: Leaving directory '/home/dmark/dev/netatalk/libatalk/iniparser'

A quick googling suggests that strlcpy and strlcat aren't part of Linux libc. This one workaround worked for me but I wonder if there's a way to not introduce a dependency on BSD libc headers?

@ghost
Copy link
Author

ghost commented Apr 7, 2023

Ifdefs have been added back. Many of the string functions seem to have their cons as well as pro's, they are deprecated in some operating systems but not in others which leads to an inconsistent picture. Not sure where to go with this one, you could always revert the iniparser commit if you'd rather leave things as they were.

The PR for commit b568955 was prepared incorrectly.
@rdmark
Copy link
Member

rdmark commented Apr 7, 2023

Never mind. The calls to strlcpy and strlcat were there even before your changesets. We can deal with the portability improvements elsewhere. Or perhaps someone more well versed in libc portability can tell us that it is a non-problem. :)

@rdmark
Copy link
Member

rdmark commented Apr 7, 2023

BTW, with "more descriptive" commit message, I meant something that objectively and independently describes the substance of the changeset. In this case, perhaps something like "add missing logic for iniparser 4.1 upgrade, and code style improvements"

@rdmark rdmark merged commit ba81b7d into Netatalk:main Apr 7, 2023
@ghost ghost deleted the iniparser branch April 8, 2023 18:28
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.

1 participant