[Bugs] [Bug 1449416] New: errno used incorrectly or misleadingly in error messages

[bugzilla at redhat.com]

https://bugzilla.redhat.com/show_bug.cgi?id=1449416

            Bug ID: 1449416
           Summary: errno used incorrectly or misleadingly in error
                    messages
           Product: GlusterFS
           Version: 3.10
         Component: core
          Assignee: bugs at gluster.org
          Reporter: nh2-redhatbugzilla at deditus.de
                CC: bugs at gluster.org



I found multiple occurrences in glusterfs where code checks `errno` following a
function that does not explicitly document that it sets errno.

In some cases this does not lead to bugs, as the code inside those functions
really does call an inner function which sets errno and whose errno makes sense
for the outer function.

In other cases though, the errno can come from inner functions that have can be
wrong or at least confusing for the outer function.

This can lead to horribly bad error message, like `Unable to end. Error :
Success` which makes no sense, and which lead me to finding this issue.

errno should only be checked after functions that explicitly declare that errno
will be set after they return.

Some set of functions that I found have this problem:

glusterd_create_status_file()

Errno checked incorrectly in:
https://github.com/gluster/glusterfs/blob/dc4aa17e617b21d9faa00dc5048e3396bde63d95/xlators/mgmt/glusterd/src/glusterd-geo-rep.c#L3841
(multiple other occurrences in this file)

It seems that the errno here can virtually come from any function, such as
gf_msg() at the end of glusterd_create_status_file() printing a debug message,
so this seems very wrong.

glusterd_is_fuse_available()

Errno checked incorrectly in:
https://github.com/gluster/glusterfs/blob/dc4aa17e617b21d9faa00dc5048e3396bde63d95/xlators/mgmt/glusterd/src/glusterd-geo-rep.c#L3616
(multiple other occurrences in this file, and one more occurrence in a
different file)

The errno here is used to generate the error message ("Unable to open /dev/fuse
%s", strerror(errno)), which is wrong because the errno doesn't come from the
open() call in glusterd_is_fuse_available() in all cases, it can also come from
the close() call.

runner_start()

Errno checked incorrectly in:
https://github.com/gluster/glusterfs/blob/dc4aa17e617b21d9faa00dc5048e3396bde63d95/xlators/mgmt/glusterd/src/glusterd-geo-rep.c#L5181
(multiple other occurrences in this file)

The errno here can easily come from sys_close() at the end of runner_start(),
thus having nothing to do with the error message that it is used in.

This function has a return code that should probably be used instead.

runner_end()

Errno checked incorrectly in:
https://github.com/gluster/glusterfs/blob/dc4aa17e617b21d9faa00dc5048e3396bde63d95/xlators/mgmt/glusterd/src/glusterd-geo-rep.c#L5227

The errno here can easily come from sys_close() at the end of runner_end(),
thus having nothing to do with the error message that it is used in.

This function has a return code that should probably be used instead.

gf_strdup()

Errno checked without documentation asserting that it is OK:
https://github.com/gluster/glusterfs/blob/dc4aa17e617b21d9faa00dc5048e3396bde63d95/xlators/mgmt/glusterd/src/glusterd-geo-rep.c#L5756
(multiple other occurrences in this file)

This use is probably not incorrect, but it doesn't gf_strdup() document that it
guarantees to set a sensible errno, so that should not be relied upon, or such
documentation should be added to the header file declaring gf_strdup().

Summary:

* 4 of the above functions seem to use errno in a way that can yield wrong or
misleading error messages, thus producing error messages like `Unable to end.
Error : Success`.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.