Error codes in fluffy.c & .h, some minor fix #8
Conversation
Neol-d2022
force-pushed
the
develop-neold2022-20180620
branch
from
766dcab
to
53694ad
Compare
There was a problem hiding this comment.
Thank you @Neol-d2022 for the PR, your efforts are much appreciated!
We may have to put a bit more thought in to this. If you are willing to take this up, please consider creating an issue with your suggestions and approach. From there on it will certainly help us converge on a solution.
Most of the returns are -1 even for the cases where errno might have been set. This was deliberately done in the interim to be handled properly later. So now that you've started to address this, let's try to do it well.
Before moving on to a more granular review, I thought it might be better to lead with general suggestions from what I observed in your patch.
For starters, in such cases where errno has been set already, it would be redundant to define custom error codes to convey that message. Doing so would be a poor violation of DRY principle.
We should probably read the errno and pass it on to the caller so that strerr() can be called on it if and when required. To do this well, we may need to place error handling functions that will lookup our custom defined error codes and strings or just call perror() when it's an error that's not set by Fluffy. This would be easier done if the error codes are enumerated rather than leaving them as #defines. Even better if we figure out a way to have the error codes and strings as a struct which can be then typedef-ed as error_t. Or, something of that sorts: https://stackoverflow.com/a/27990368/2877507 & https://stackoverflow.com/a/10966395/2877507 .
Just to be sure that things are clear, in case you are wondering why the need to pass on the errno when it's been set already and available for access, I've left a relevant comment some where down below for FLUFFY_ERROR_OPEN_QUEUE_FAILED.
Also, strings like perror("inotify_add_watch in dir_tree_add_watch(), FILE: fluffly.c"); is not a standard way to report errors. If your rationale behind that approach was to aid with the debugging(file and function names), don't worry, gdb is there for the rescue!
| @@ -85,6 +118,7 @@ struct fluffy_event_info { | |||
| * - int: 0 to continue with the next event processing, | |||
| * any other integer(preferably -1) to destroy the context | |||
| */ | |||
| // TODO: fluffy_init actually returns flhandle (a positive integer) when successful (see the implementation). Edit the function description? (neold2022) | |||
There was a problem hiding this comment.
Good eye. You are right, it needs correction. This is a 'copy paste' mistake from the description of fluffy_print_event() I suppose.
Returns a positive context handle when successful.
There was a problem hiding this comment.
Returns a positive context handle when successful.
Yup, I got this one.
| } | ||
|
|
||
| fd = open("/proc/sys/fs/inotify/max_user_instances", | ||
| O_WRONLY | O_TRUNC | O_CLOEXEC); | ||
| if (fd == -1) { | ||
| return -1; | ||
| return FLUFFY_ERROR_OPEN_QUEUE_FAILED; |
There was a problem hiding this comment.
Returning FLUFFY_ERROR_OPEN_QUEUE_FAILED does not actually get the message across because errno is transient in nature. This might caution the user that opening the file failed but no means of knowing the cause behind it. errno holds the cause/reason behind the failure but its value is likely to change by the time the user reads errno because of its transient nature.
On a relevant note, defining a Fluffy error makes sense only when the context is solely in relevance with it. For all other cases we must hand off what ever the standard library calls return.
There was a problem hiding this comment.
For starters, in such cases where errno has been set already, it would be redundant to define custom error codes to convey that message. Doing so would be a poor violation of DRY principle.
On a relevant note, defining a Fluffy error makes sense only when the context is solely in relevance with it. For all other cases we must hand off what ever the standard library calls return.
Omg, @raamsri you are so right, I'm on it.
| } | ||
|
|
||
| /* Get the fluffy_context_info of this handle */ | ||
| struct fluffy_context_info *ctxinfop; | ||
| ctxinfop = fluffy_get_context_info(flhandle); | ||
| if (ctxinfop == NULL) { | ||
| return -1; | ||
| return -FLUFFY_ERROR_CTXINFOP; |
There was a problem hiding this comment.
It's misleading when a minus sign is prefixed. If the value is a negative constant, it's better to define a negative integer.
| #define FLUFFY_ERROR_INVALID_ARG 1 | ||
| #define FLUFFY_ERROR_OPEN_QUEUE_FAILED 2 /* errno is set */ | ||
| #define FLUFFY_ERROR_WRITE_QUEUE_FAILED 3 /* errno is set */ | ||
| #define FLUFFY_ERROR_CLOSE_QUEUE_FAILED 4 /* errno is set */ |
There was a problem hiding this comment.
Fancy a shorter prefix like EF_*? Also, adding 'FAILED' seems redundant.
Hi all,
I have done some modifications (most of them is about error code reporting), please review them.
Also, at the end of the function
static int fluffy_handoff_event()influffy.c, it returns whatever client has returned. Note that the value returned by clients could accidentally collide with error codes (recently) defined influffy.h.