[WIP] libsubid: don't print error messages on stderr by default #335
Conversation
|
Drat, I meant this to be a 'draft' pull request. Only compile-tested so far. |
hallyn
changed the title
|
oh neato, a new 'convert to draft' button. |
| /* | ||
| * libsubid_init: initialize libsubid | ||
| * | ||
| * @progname: Name to display as program. If NULL, then "(libsubid)" will be |
There was a problem hiding this comment.
I would mention pointer is kept (not memory copy). Thus caller should not release memory.
There was a problem hiding this comment.
Hm, actually I'm doing an strdup now instead. Prefer not to risk segfaults due to an accidental caller garbage collection.
libsubid/api.c
Outdated
| shadow_logfd = logfd; | ||
| return; | ||
| } | ||
| shadow_logfd = fopen("/dev/null", O_RDWR); |
| * shown in error messages. | ||
| * @logfd: Open file pointer to pass error messages to. If NULL, then | ||
| * /dev/null will be opened and messages will be sent there. The | ||
| * default if libsubid_init() is not called is stderr (2). |
There was a problem hiding this comment.
Can we have "/dev/null" as a default? I think it makes perfect sense for a lib.
There was a problem hiding this comment.
That's more difficult in this approach. Would have to use an __init, which I'd prefer not to. When we switch to a proper log subsystem, we can make that change.
| } | ||
| shadow_logfd = fopen("/dev/null", O_RDWR); | ||
| if (!shadow_logfd) { | ||
| fprintf(stderr, "ERROR opening /dev/null for error messages. Using stderr."); |
There was a problem hiding this comment.
I'm not sure this is expected if caller explicitly requested "dev/null".
Perhaps error return code for init() function would make more sense here.
There was a problem hiding this comment.
I'll return false on error to help the library figure out what's going on.
There was a problem hiding this comment.
Thanks for the update with error code.
I think return false should be enough here.
IMO, fprintf() in this case is unnecessary and maybe even undesirable (taking into account "/dev/null" was requested explicitly).
libsubid/api.c
Outdated
| @@ -38,6 +38,25 @@ | |||
| #include "idmapping.h" | |||
| #include "subid.h" | |||
|
|
|||
| char *Prog = "(libsubid)"; | |||
|
Hi @hallyn, since your draft touches every single But general approach is up to you, of course. I don't object keeping it this way. |
|
@alexey-tikhonov I agree with your feeling that a proper log subsystem would be nicer, and I intend to do that "next". But this was doable with mostly just a 'sed' and no scary complications. As I think you're under a time pressure to release the package, this should suffice for now, right? |
Closes shadow-maint#325 Add a new subid_init() function which can be used to specify the stream on which error messages should be printed. (If you want to get fancy you can redirect that to memory :) If subid_init() is not called, use stderr. If NULL is passed, then /dev/null will be used. This patch also fixes up the 'Prog', which previously had to be defined by any program linking against libsubid. Now, by default in libsubid it will show (subid). Once subid_init() is called, it will use the first variable passed to subid_init(). Signed-off-by: Serge Hallyn <[email protected]>
| if (progname) | ||
| Prog = progname; | ||
| else | ||
| fprintf(stderr, "Out of memory"); |
|
Yes, this would suffice. Thank you, @hallyn. (Please take a note I didn't review changes other than in |
Closes #325
Add a new subid_init() function which can be used to specify the
stream on which error messages should be printed. (If you want to
get fancy you can redirect that to memory :) If subid_init() is
not called, use stderr. If NULL is passed, then /dev/null will
be used.
This patch also fixes up the 'Prog', which previously had to be
defined by any program linking against libsubid. Now, by default
in libsubid it will show (subid). Once subid_init() is called,
it will use the first variable passed to subid_init().
Signed-off-by: Serge Hallyn [email protected]