-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
Remove use of getcwd for resolving paths #242
Conversation
Call fopen with name rather than resolved path to allow use of specialized symbolic links for pipes and sockets. E.g., /dev/stdin. Fixes NLnetLabs/nsd#380.
a63103f
to
b4fcb99
Compare
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.
Hi @k0ekk0ek,
Sorry for not sending the contribution myself like I promised. Thanks for fixing this :)
I can't comment on the Win32 code branches, but I did a proof read of the Unix code path. It's the least I can do and I hope you don't mind.
I suggested some minor improvements.
Have a great day, David
|
||
if (!resolved) | ||
if (!(resolved = realpath(include, buffer))) | ||
return (errno == ENOMEM) ? ZONE_OUT_OF_MEMORY : ZONE_NOT_A_FILE; |
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.
This is significant improvement over the original error handling. Just a small nitpick: this can still translate e.g. EACCESS to "no such file", which is probably going to be confusing. But I guess this is not something that can be fixed easily.
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.
It'd be pretty easy to add an additional code, but I mainly wanted to distinguish between no memory being available and not being able to open the file figuring further distinctions wouldn't make all that much sense(?) As in, we don't check for them in NSD, but other users of the library might (though I'm not aware of any such users yet).
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.
Ah, I see :) that makes sense. Perhaps it's just the name that confused me (ZONE_NOT_A_FILE). Maybe something like "ZONE_OPEN_FAILED" would be more descriptive? Because that includes all the other cases :)
Thanks for reviewing @dcepelik 👍 |
@wtoorop, I simply assumed you agree with the changes so I can issue a follow-up PR 😅. |
Call fopen with name rather than resolved path to allow use of specialized symbolic links for pipes and sockets. E.g., /dev/stdin.
Fixes NLnetLabs/nsd#380.