Hello,
I recently started working on ticket #1991. https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think that the core of the problem is the expectation that ldb_search returns LDB_ERR_NO_SUCH_OBJECT in case no results were found. But it instead returns LDB_SUCCESS and sets result->count to 0.
I think it would be best for sysdb functions in case that no results were found to return both ENOENT value and 'properly' empty list.
Thanks for opinions.
Pavel Reichl
On Tue, Apr 22, 2014 at 11:31:31AM +0200, Pavel Reichl wrote:
Hello,
I recently started working on ticket #1991. https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think that the core of the problem is the expectation that ldb_search returns LDB_ERR_NO_SUCH_OBJECT in case no results were found. But it instead returns LDB_SUCCESS and sets result->count to 0.
I think it would be best for sysdb functions in case that no results were found to return both ENOENT value and 'properly' empty list.
Thanks for opinions.
Pavel Reichl
I agree with returning an error code rather than an empty list. The empty list has bitten us in the past already, the return code would make sure the programmer has to explicitly handle it.
I wonder if we could go even further and introduce a new error code (ERR_NO_SUCH_OBJECT). ENOENT is used quite a bit in the code, so the new error code might be harder to convert to: $ git grep ENOENT | wc -l 756
On (22/04/14 11:39), Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 11:31:31AM +0200, Pavel Reichl wrote:
Hello,
I recently started working on ticket #1991. https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think that the core of the problem is the expectation that ldb_search returns LDB_ERR_NO_SUCH_OBJECT in case no results were found. But it instead returns LDB_SUCCESS and sets result->count to 0.
I think it would be best for sysdb functions in case that no results were found to return both ENOENT value and 'properly' empty list.
Thanks for opinions.
Pavel Reichl
I agree with returning an error code rather than an empty list. The empty list has bitten us in the past already, the return code would make sure the programmer has to explicitly handle it.
I wonder if we could go even further and introduce a new error code
New error code will make sense if you want more precise expression of error.
(ERR_NO_SUCH_OBJECT). ENOENT is used quite a bit in the code, so the
What will be difference between ENOENT and ERR_NO_SUCH_OBJECT? for example: ERR_MISSING_CONF
new error code might be harder to convert to: $ git grep ENOENT | wc -l 756
LS
On Tue, Apr 22, 2014 at 12:31:36PM +0200, Lukas Slebodnik wrote:
On (22/04/14 11:39), Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 11:31:31AM +0200, Pavel Reichl wrote:
Hello,
I recently started working on ticket #1991. https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think that the core of the problem is the expectation that ldb_search returns LDB_ERR_NO_SUCH_OBJECT in case no results were found. But it instead returns LDB_SUCCESS and sets result->count to 0.
I think it would be best for sysdb functions in case that no results were found to return both ENOENT value and 'properly' empty list.
Thanks for opinions.
Pavel Reichl
I agree with returning an error code rather than an empty list. The empty list has bitten us in the past already, the return code would make sure the programmer has to explicitly handle it.
I wonder if we could go even further and introduce a new error code
New error code will make sense if you want more precise expression of error.
(ERR_NO_SUCH_OBJECT). ENOENT is used quite a bit in the code, so the
What will be difference between ENOENT and ERR_NO_SUCH_OBJECT? for example: ERR_MISSING_CONF
1) strerror would report better error string 2) ENOENT can be returned by other underlying function (and therefore could be treated as a real error) , ERR_NO_SUCH_OBJECT would only be returned by sysdb.
On Tue, 2014-04-22 at 13:09 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 12:31:36PM +0200, Lukas Slebodnik wrote:
On (22/04/14 11:39), Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 11:31:31AM +0200, Pavel Reichl wrote:
Hello,
I recently started working on ticket #1991. https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think that the core of the problem is the expectation that ldb_search returns LDB_ERR_NO_SUCH_OBJECT in case no results were found. But it instead returns LDB_SUCCESS and sets result->count to 0.
I think it would be best for sysdb functions in case that no results were found to return both ENOENT value and 'properly' empty list.
Thanks for opinions.
Pavel Reichl
I agree with returning an error code rather than an empty list. The empty list has bitten us in the past already, the return code would make sure the programmer has to explicitly handle it.
I wonder if we could go even further and introduce a new error code
New error code will make sense if you want more precise expression of error.
(ERR_NO_SUCH_OBJECT). ENOENT is used quite a bit in the code, so the
What will be difference between ENOENT and ERR_NO_SUCH_OBJECT? for example: ERR_MISSING_CONF
- strerror would report better error string
- ENOENT can be returned by other underlying function (and therefore
could be treated as a real error) , ERR_NO_SUCH_OBJECT would only be returned by sysdb.
So are we really going with ERR_NO_SUCH_OBJECT? I don't say it's a bad idea. I'm just afraid it may be too much work.
If we'll just make sure that ENOENT is returned if no result is found we have to change just the implementation of some sysdb functions and in code that calls them.
But if we go with new return value we'll have to change implementation of ALL (relevant) sysdb functions and *ALL* code that is calling them. It may prove difficult to trace the 'journey' of ENOENT return value from sysdb functions through calling hierarchy. I think that this would create a potential for introduction many new bugs.
On Mon, Apr 28, 2014 at 02:31:36PM +0200, Pavel Reichl wrote:
On Tue, 2014-04-22 at 13:09 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 12:31:36PM +0200, Lukas Slebodnik wrote:
On (22/04/14 11:39), Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 11:31:31AM +0200, Pavel Reichl wrote:
Hello,
I recently started working on ticket #1991. https://fedorahosted.org/sssd/ticket/1991
"The return codes of various sysdb operations differ. Some search operations would return ENOENT if they don't find a matching object some would return EOK but an empty result list."
I think that the core of the problem is the expectation that ldb_search returns LDB_ERR_NO_SUCH_OBJECT in case no results were found. But it instead returns LDB_SUCCESS and sets result->count to 0.
I think it would be best for sysdb functions in case that no results were found to return both ENOENT value and 'properly' empty list.
Thanks for opinions.
Pavel Reichl
I agree with returning an error code rather than an empty list. The empty list has bitten us in the past already, the return code would make sure the programmer has to explicitly handle it.
I wonder if we could go even further and introduce a new error code
New error code will make sense if you want more precise expression of error.
(ERR_NO_SUCH_OBJECT). ENOENT is used quite a bit in the code, so the
What will be difference between ENOENT and ERR_NO_SUCH_OBJECT? for example: ERR_MISSING_CONF
- strerror would report better error string
- ENOENT can be returned by other underlying function (and therefore
could be treated as a real error) , ERR_NO_SUCH_OBJECT would only be returned by sysdb.
So are we really going with ERR_NO_SUCH_OBJECT? I don't say it's a bad idea. I'm just afraid it may be too much work.
If we'll just make sure that ENOENT is returned if no result is found we have to change just the implementation of some sysdb functions and in code that calls them.
But if we go with new return value we'll have to change implementation of ALL (relevant) sysdb functions and *ALL* code that is calling them. It may prove difficult to trace the 'journey' of ENOENT return value from sysdb functions through calling hierarchy. I think that this would create a potential for introduction many new bugs.
Yeah, you might be right...I haven't done any prototyping, I was just assuming that error codes from sysdb functions never bubble up but are dealt with by the caller. I guess you know the problem space better since you already spent some time with the sysdb code lately.
Anyway, even if we were changing the errno codes, it would be a separate patchset. So I'd say we should go ahead with the current plan, change only returning ENOENT vs EOK+empty set and worry about errno codes later.
But in general, I think the specific error codes make sense, especially when dealing with code that is higher up than system level..for instance in the DBus responder, (ab)using the standard errno codes only leads to code full of EIOs and EFAULTs so there I'm trying to use new error codes where possible..
On Mon, 2014-04-28 at 21:06 +0200, Jakub Hrozek wrote:
On Mon, Apr 28, 2014 at 02:31:36PM +0200, Pavel Reichl wrote:
On Tue, 2014-04-22 at 13:09 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 12:31:36PM +0200, Lukas Slebodnik wrote:
On (22/04/14 11:39), Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 11:31:31AM +0200, Pavel Reichl wrote:
[snip]
I think it would be best for sysdb functions in case that no results were found to return both ENOENT value and 'properly' empty list.
[snip]
So I'd say we should go ahead with the current plan, change only returning ENOENT vs EOK+empty set and worry about errno codes later.
OK, so if ENOENT is returned do we guarantee that output parameters are valid and 'output count' is set to 0?
On Wed, May 28, 2014 at 09:31:28AM +0200, Pavel Reichl wrote:
On Mon, 2014-04-28 at 21:06 +0200, Jakub Hrozek wrote:
On Mon, Apr 28, 2014 at 02:31:36PM +0200, Pavel Reichl wrote:
On Tue, 2014-04-22 at 13:09 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 12:31:36PM +0200, Lukas Slebodnik wrote:
On (22/04/14 11:39), Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 11:31:31AM +0200, Pavel Reichl wrote:
[snip]
> I think it would be best for sysdb functions in case that no results > were found to return both ENOENT value and 'properly' empty list.
[snip]
So I'd say we should go ahead with the current plan, change only returning ENOENT vs EOK+empty set and worry about errno codes later.
OK, so if ENOENT is returned do we guarantee that output parameters are valid and 'output count' is set to 0?
I'm not concerned about separate count, that's usually not the issue.
Looking at the sysdb API, we seem to be returning either ldb_message or ldb_result. I think the original issue was that we could have returned EOK but either let the pointer to ldb_message undefined or NULL, I'm not sure which it was.
Then we had crashes in code that looked perfectly valid, like:
struct ldb_message *msg;
ret = sysdb_search(sysdb, filter, &msg); if (ret != EOK) goto fail;
for (int i = 0; i < msg->num_elements; i++) { do_stuff(); }
Dereferencing msg was the problem.
And personally I don't think we should guarantee setting msg to anything on return code != EOK. Just let the caller handle the return code..
On (28/05/14 10:14), Jakub Hrozek wrote:
On Wed, May 28, 2014 at 09:31:28AM +0200, Pavel Reichl wrote:
On Mon, 2014-04-28 at 21:06 +0200, Jakub Hrozek wrote:
On Mon, Apr 28, 2014 at 02:31:36PM +0200, Pavel Reichl wrote:
On Tue, 2014-04-22 at 13:09 +0200, Jakub Hrozek wrote:
On Tue, Apr 22, 2014 at 12:31:36PM +0200, Lukas Slebodnik wrote:
On (22/04/14 11:39), Jakub Hrozek wrote: >On Tue, Apr 22, 2014 at 11:31:31AM +0200, Pavel Reichl wrote:
[snip]
>> I think it would be best for sysdb functions in case that no results >> were found to return both ENOENT value and 'properly' empty list.
[snip]
So I'd say we should go ahead with the current plan, change only returning ENOENT vs EOK+empty set and worry about errno codes later.
OK, so if ENOENT is returned do we guarantee that output parameters are valid and 'output count' is set to 0?
I'm not concerned about separate count, that's usually not the issue.
Looking at the sysdb API, we seem to be returning either ldb_message or ldb_result. I think the original issue was that we could have returned EOK but either let the pointer to ldb_message undefined or NULL, I'm not sure which it was.
Then we had crashes in code that looked perfectly valid, like:
struct ldb_message *msg;
ret = sysdb_search(sysdb, filter, &msg); if (ret != EOK) goto fail;
for (int i = 0; i < msg->num_elements; i++) { do_stuff(); }
Dereferencing msg was the problem.
And personally I don't think we should guarantee setting msg to anything on return code != EOK. Just let the caller handle the return code.
+1
LS
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Reichl