-
-
Notifications
You must be signed in to change notification settings - Fork 762
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
dprintf() polyfill for Illumos #1911
Conversation
int len, nwritten; | ||
|
||
va_start(ap, format); | ||
len = vasprintf(&s, format, ap); |
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.
Although posix doesn't require it, the general expectation is for dprintf
to not do any memory allocation so that it can be used in signal handler context. Not doing allocation would also remove allocation related failure point.
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.
I agree it would be better but I couldn't find a better alternative with the available primitives. One could use a static buffer but then you're length limited.
@@ -121,6 +121,10 @@ | |||
#define alloca(size) __builtin_alloca(size) | |||
#endif | |||
|
|||
#ifdef __sun |
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.
If Illumos is maintained, why don't they add dprintf
? It's more economical for them to add it instead of N projects to carry their own implementation.
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.
Would be better if they did indeed. But that does leave legacy systems, and even there nnn can be very useful.
@N-R-K please merge when ready. |
Turns out there aren't that many dprintf() calls so perhaps this alternative is better: #1912 |
This the improved version submitted to upstream: jarun/nnn#1911
Closed with #1912. |
I realise it's a bit of a long shot, submitting a PR to fix a legacy platform, but I thought I might at least try upstreaming this patch we've been using for a while.
Edit: this is also for Illumos, which is actively maintained.