make posix return errors when gfid2path key is absent

Change-Id: I3a8d452d00560dac5e0b7ff0b1835d1f20a59f91
updates: bz#1580540
Signed-off-by: Raghavendra Bhat <raghavendra@redhat.com>
(cherry picked from commit c2cf3f686f3ea0efd936d2eafc404fc9d2e0acc7)

5 files changed, 127 insertions, 26 deletions

diff --git a/libglusterfs/src/syncop-utils.c b/libglusterfs/src/syncop-utils.c
index 40ced03cb45..1ccda55e4c5 100644
--- a/libglusterfs/src/syncop-utils.c
+++ b/libglusterfs/src/syncop-utils.c
@@ -591,9 +591,21 @@ syncop_gfid_to_path_hard (inode_table_t *itable, xlator_t *subvol, uuid_t gfid,
         if (ret < 0)
                 goto out;
 
-        ret = dict_get_str (xattr, hard_resolve ?
-                            GFID2PATH_VIRT_XATTR_KEY : GFID_TO_PATH_KEY,
-                            &path);
+
+        /*
+         * posix will do dict_set_dynstr for GFID_TO_PATH_KEY i.e.
+         * for in memory search for the path. And for on disk xattr
+         * fetching of the path for the key GFID2PATH_VIRT_XATTR_KEY
+         * it uses dict_set_dynptr. So, for GFID2PATH_VIRT_XATTR_KEY
+         * use dict_get_ptr to avoid dict complaining about type
+         * mismatch (i.e. str vs ptr)
+         */
+        if (!hard_resolve)
+                ret = dict_get_str (xattr, GFID_TO_PATH_KEY, &path);
+        else
+                ret = dict_get_ptr (xattr, GFID2PATH_VIRT_XATTR_KEY,
+                                    (void **)&path);
+
         if (ret || !path) {
                 ret = -EINVAL;
                 goto out;
diff --git a/tests/bugs/posix/bug-gfid-path.t b/tests/bugs/posix/bug-gfid-path.t
new file mode 100644
index 00000000000..1bbbe9f0670
--- /dev/null
+++ b/tests/bugs/posix/bug-gfid-path.t
@@ -0,0 +1,70 @@
+#!/bin/bash
+
+# This test case is for the bug where, even though a file is
+# created when gfid2path option is turned off (default is ON),
+# getfattr of "glusterfs.gfidtopath" was succeeding for that
+# file. Ideally the getfattr should fail, as the file does not
+# have its path(s) stored as a extended attribute (because it
+# was created when gfid2path option was off)
+
+. $(dirname $0)/../../include.rc
+. $(dirname $0)/../../volume.rc
+
+cleanup;
+
+TEST glusterd
+TEST pidof glusterd
+TEST $CLI volume info;
+
+TEST $CLI volume create $V0 $H0:$B0/${V0}{1,2,3,4};
+
+EXPECT "$V0" volinfo_field $V0 'Volume Name';
+EXPECT 'Created' volinfo_field $V0 'Status';
+EXPECT '4' brick_count $V0
+
+TEST $CLI volume start $V0;
+EXPECT 'Started' volinfo_field $V0 'Status';
+
+TEST glusterfs -s $H0 --volfile-id $V0 $M0;
+
+TEST mkdir $M0/dir
+TEST mkdir $M0/new
+TEST mkdir $M0/3
+
+TEST touch $M0/dir/file
+
+# except success as by default gfid2path is enabled
+# and the glusterfs.gfidtopath xattr should give the
+# path of the object as the value
+
+TEST getfattr -n glusterfs.gfidtopath $M0/dir/file
+
+# turn off gfid2path feature
+TEST $CLI volume set $V0 storage.gfid2path off
+
+TEST touch $M0/new/foo
+
+# again enable gfid2path. This has to be enabled before
+# trying the getfattr. Because, glusterfs.gfidtopath xattr
+# request is handled only if gfid2path is enabled. If not,
+# then getxattr on glusterfs.gfid2path fails anyways. In this
+# context we want getfattr to fail, because the file was created
+# when gfid2path feature was disabled and not because gfid2path
+# feature itself is disabled.
+TEST $CLI volume set $V0 storage.gfid2path on
+
+# getfattr should fail as it is attempted on a file
+# which does not have its path stored as a xattr
+# (because file got created after disabling gfid2path)
+TEST ! getfattr -n glusterfs.gfidtopath $M0/new/foo;
+
+
+
+TEST touch $M0/3/new
+
+# should be successful
+TEST getfattr -n glusterfs.gfidtopath $M0/3/new
+
+TEST rm -rf $M0/*
+
+cleanup;
diff --git a/xlators/features/bit-rot/src/stub/bit-rot-stub-helpers.c b/xlators/features/bit-rot/src/stub/bit-rot-stub-helpers.c
index 24aa9aa5ff4..fff7a7eedf2 100644
--- a/xlators/features/bit-rot/src/stub/bit-rot-stub-helpers.c
+++ b/xlators/features/bit-rot/src/stub/bit-rot-stub-helpers.c
@@ -722,35 +722,21 @@ br_stub_get_path_of_gfid (xlator_t *this, inode_t *parent, inode_t *inode,
         GF_VALIDATE_OR_GOTO (this->name, parent, out);
         GF_VALIDATE_OR_GOTO (this->name, path, out);
 
-        /* No need to validate the @inode for hard resolution. Because inode
-         * can be NULL and if it is NULL, then syncop_gfid_to_path_hard will
-         * allocate a new inode and proceed. So no need to bother about
+        /* Above, No need to validate the @inode for hard resolution. Because
+         * inode can be NULL and if it is NULL, then syncop_gfid_to_path_hard
+         * will allocate a new inode and proceed. So no need to bother about
          * @inode. Because we need it only to send a syncop_getxattr call
          * from inside syncop_gfid_to_path_hard. And getxattr fetches the
          * path from the backend.
          */
+
         ret = syncop_gfid_to_path_hard (parent->table, FIRST_CHILD (this), gfid,
                                         inode, path, _gf_true);
-
-        /*
-         * This is to handle those corrupted files which does not contain
-         * the gfid2path xattr in the backend (because they were created
-         * when the option was OFF OR it was upgraded from a version before
-         * gfid2path was brought in.
-         * Ideally posix should be returning ret < 0 i.e. error if the
-         * gfid2path xattr is not present. But for some reason it is
-         * returning success and path as "". THis is causing problems.
-         * For now handling it by adding extra checks. But the better way
-         * is to make posix return error if gfid2path xattr is absent.
-         * When that is done remove below if block and also this entire
-         * comment.
-         */
-        if (ret >= 0 && !strlen (*path)) {
+        if (ret < 0)
                 gf_msg (this->name, GF_LOG_WARNING, 0, BRS_MSG_PATH_GET_FAILED,
-                        "path for the object %s is %s. Going for in memory path",
-                        uuid_utoa_r (gfid, gfid_str), *path);
-                ret = -1;
-        }
+                        "failed to get the path xattr from disk for the "
+                        " gfid %s. Trying to get path from the memory",
+                        uuid_utoa_r (gfid, gfid_str));
 
         /*
          * Try with soft resolution of path if hard resolve fails. Because
@@ -767,10 +753,16 @@ br_stub_get_path_of_gfid (xlator_t *this, inode_t *parent, inode_t *inode,
          * found in the inode table and better not to do inode_path() on the
          * inode which has not been linked.
          */
-        if (ret < 0 && inode)
+        if (ret < 0 && inode) {
                 ret = syncop_gfid_to_path_hard (parent->table,
                                                 FIRST_CHILD (this), gfid, inode,
                                                 path, _gf_false);
+                if (ret < 0)
+                        gf_msg (this->name, GF_LOG_WARNING, 0,
+                                BRS_MSG_PATH_GET_FAILED,
+                                "failed to get the path from the memory for gfid %s",
+                                uuid_utoa_r (gfid, gfid_str));
+        }
 
 out:
         return ret;
diff --git a/xlators/storage/posix/src/posix-gfid-path.c b/xlators/storage/posix/src/posix-gfid-path.c
index 2834c2fc4fd..9873e7e5a84 100644
--- a/xlators/storage/posix/src/posix-gfid-path.c
+++ b/xlators/storage/posix/src/posix-gfid-path.c
@@ -123,6 +123,7 @@ posix_get_gfid2path (xlator_t *this, inode_t *inode, const char *real_path,
         gf_boolean_t           have_val                        = _gf_false;
         struct posix_private  *priv                            = NULL;
         char                   pargfid_str[UUID_CANONICAL_FORM_LEN + 1] = {0,};
+        gf_boolean_t           found                           = _gf_false;
 
         priv = this->private;
 
@@ -141,6 +142,7 @@ posix_get_gfid2path (xlator_t *this, inode_t *inode, const char *real_path,
                                 "value for key (%s)", GFID2PATH_VIRT_XATTR_KEY);
                         goto err;
                 }
+                found = _gf_true;
         } else {
                 have_val = _gf_false;
                 memset (value_buf, '\0', sizeof(value_buf));
@@ -202,6 +204,7 @@ posix_get_gfid2path (xlator_t *this, inode_t *inode, const char *real_path,
                                 goto ignore;
                         }
 
+                        found = _gf_true;
                         memset (value_buf, '\0', sizeof(value_buf));
                         size = sys_lgetxattr (real_path, keybuffer, value_buf,
                                               sizeof (value_buf) - 1);
@@ -231,6 +234,29 @@ ignore:
                         list_offset += strlen (keybuffer) + 1;
                 } /* while (remaining_size > 0) */
 
+                /* gfid2path xattr is absent in the list of xattrs */
+                if (!found) {
+                        ret = -1;
+                        /*
+                         * ENODATA because xattr is not present in the
+                         * list of xattrs. Thus the consumer should
+                         * face error instead of a success and a empty
+                         * string in the dict for the key.
+                         */
+                        *op_errno = ENODATA;
+                        goto err;
+                }
+
+                /*
+                 * gfid2path xattr is found in list of xattrs, but getxattr
+                 * on the 1st gfid2path xattr itself failed and the while
+                 * loop above broke. So there is nothing in the value. So
+                 * it would be better not to send "" as the value for any
+                 * key, as it is not true.
+                 */
+                if (found && !i)
+                        goto err; /* both errno and ret are set before beak */
+
                 /* Calculate memory to be allocated */
                 for (j = 0; j < i; j++) {
                         bytes += strlen(paths[j]);
diff --git a/xlators/storage/posix/src/posix-gfid-path.h b/xlators/storage/posix/src/posix-gfid-path.h
index ac3d03e14c3..59799125bbb 100644
--- a/xlators/storage/posix/src/posix-gfid-path.h
+++ b/xlators/storage/posix/src/posix-gfid-path.h
@@ -13,6 +13,7 @@
 
 #include "xlator.h"
 #include "common-utils.h"
+#include "compat-errno.h"
 
 #define MAX_GFID2PATH_LINK_SUP 500