cluster/ec: Fix dictionary compare function

If both dicts are NULL then equal. If one of the dicts is NULL but the other
has only ignorable keys then also they are equal. If both dicts are non-null
then check if for each non-ignorable key, values are same or not.  value_ignore
function is used to skip comparing values for the keys which must be present in
both the dictionaries but the value could be different.

geo-rep's stime xattr doesn't need to be present in list xattr but when
getxattr comes on stime xattr even if there aren't enough responses with the
xattr we should still give out an answer which is maximum of the stimes
available.

Change-Id: I8de2ceaa2db785b797f302f585d88e73b154167d
BUG: 1207712
Signed-off-by: Pranith Kumar K <pkarampu@redhat.com>
Reviewed-on: http://review.gluster.org/10078
Tested-by: Gluster Build System <jenkins@build.gluster.com>
Reviewed-by: Xavier Hernandez <xhernandez@datalab.es>

7 files changed, 152 insertions, 119 deletions

diff --git a/libglusterfs/src/dict.c b/libglusterfs/src/dict.c
index b8b6aeab248..ffc92e72724 100644
--- a/libglusterfs/src/dict.c
+++ b/libglusterfs/src/dict.c
@@ -31,6 +31,11 @@
 #include "globals.h"
 #include "statedump.h"
 
+struct dict_cmp {
+        dict_t *dict;
+        gf_boolean_t (*value_ignore) (char *k);
+};
+
 data_t *
 get_new_data ()
 {
@@ -107,7 +112,6 @@ dict_new (void)
         return dict;
 }
 
-
 int32_t
 is_data_equal (data_t *one,
                data_t *two)
@@ -134,6 +138,87 @@ is_data_equal (data_t *one,
         return 0;
 }
 
+static int
+key_value_cmp (dict_t *one, char *key1, data_t *value1, void *data)
+{
+        struct dict_cmp *cmp = data;
+        dict_t *two = NULL;
+        data_t *value2 = NULL;
+
+        two = cmp->dict;
+        value2 = dict_get (two, key1);
+
+        if (value2) {
+                if (cmp->value_ignore && cmp->value_ignore (key1))
+                        return 0;
+
+                if (is_data_equal (value1, value2) == 1)
+                        return 0;
+        }
+
+        if (value2 == NULL) {
+                gf_log (THIS->name, GF_LOG_DEBUG,
+                        "'%s' found only on one dict", key1);
+        } else {
+                gf_log (THIS->name, GF_LOG_DEBUG, "'%s' is different in two "
+                        "dicts (%u, %u)", key1, value1->len, value2->len);
+        }
+
+        return -1;
+}
+
+/* If both dicts are NULL then equal. If one of the dicts is NULL but the
+ * other has only ignorable keys then also they are equal. If both dicts are
+ * non-null then check if for each non-ignorable key, values are same or
+ * not.  value_ignore function is used to skip comparing values for the keys
+ * which must be present in both the dictionaries but the value could be
+ * different.
+ */
+gf_boolean_t
+are_dicts_equal (dict_t *one, dict_t *two,
+                 gf_boolean_t (*match) (dict_t *d, char *k, data_t *v,
+                                        void *data),
+                 gf_boolean_t (*value_ignore) (char *k))
+{
+        int     num_matches1 = 0;
+        int     num_matches2 = 0;
+        struct  dict_cmp cmp = {0};
+
+        if (one == two)
+                return _gf_true;
+
+        if (!match)
+                match = dict_match_everything;
+
+        cmp.dict = two;
+        cmp.value_ignore = value_ignore;
+        if (!two) {
+                num_matches1 = dict_foreach_match (one, match, NULL,
+                                                   dict_null_foreach_fn, NULL);
+                goto done;
+        } else {
+                num_matches1 = dict_foreach_match (one, match, NULL,
+                                                   key_value_cmp, &cmp);
+        }
+
+        if (num_matches1 == -1)
+                return _gf_false;
+
+        if ((num_matches1 == one->count) && (one->count == two->count))
+                return _gf_true;
+
+        num_matches2 = dict_foreach_match (two, match, NULL,
+                                           dict_null_foreach_fn, NULL);
+done:
+        /* If the number of matches is same in 'two' then for all the
+         * valid-keys that exist in 'one' the value matched and no extra valid
+         * keys exist in 'two' alone. Otherwise there exists at least one extra
+         * valid-key in 'two' which doesn't exist in 'one' */
+        if (num_matches1 == num_matches2)
+                return _gf_true;
+        return _gf_false;
+}
+
 void
 data_destroy (data_t *data)
 {
diff --git a/libglusterfs/src/dict.h b/libglusterfs/src/dict.h
index 3708eede06d..a9004e96a50 100644
--- a/libglusterfs/src/dict.h
+++ b/libglusterfs/src/dict.h
@@ -264,4 +264,9 @@ dict_match_everything (dict_t *d, char *k, data_t *v, void *data);
 dict_t *
 dict_for_key_value (const char *name, const char *value, size_t size);
 
+gf_boolean_t
+are_dicts_equal (dict_t *one, dict_t *two,
+                 gf_boolean_t (*match) (dict_t *d, char *k, data_t *v,
+                                        void *data),
+                 gf_boolean_t (*value_ignore) (char *k));
 #endif
diff --git a/tests/basic/geo-replication/marker-xattrs.t b/tests/basic/geo-replication/marker-xattrs.t
index 7061b4532a3..dd5483d7e95 100755
--- a/tests/basic/geo-replication/marker-xattrs.t
+++ b/tests/basic/geo-replication/marker-xattrs.t
@@ -59,12 +59,16 @@ TEST touch $M0
 
 vol_uuid=$(get_volume_mark $M1)
 xtime=trusted.glusterfs.$vol_uuid.xtime
+stime=trusted.glusterfs.$vol_uuid.stime
 
+stime_val=$(getfattr -e hex -n $xtime $M1 | grep ${xtime}= | cut -f2 -d'=')
+TEST "setfattr -n $stime -v $stime_val $B0/${V0}-1"
 TEST "getfattr -n $xtime $M1 | grep -q ${xtime}="
 
 TEST kill_brick $V0 $H0 $B0/${V0}-0
 
 TEST "getfattr -n $xtime $M1 | grep -q ${xtime}="
+TEST "getfattr -n $stime $M1 | grep -q ${stime}="
 
 TEST getfattr -d -m. -e hex $M1
 EXPECT_WITHIN $UMOUNT_TIMEOUT "Y" force_umount $M0
diff --git a/xlators/cluster/afr/src/afr-common.c b/xlators/cluster/afr/src/afr-common.c
index 5654e3ad03d..8993b164b91 100644
--- a/xlators/cluster/afr/src/afr-common.c
+++ b/xlators/cluster/afr/src/afr-common.c
@@ -1239,48 +1239,19 @@ afr_is_xattr_ignorable (char *key)
         return _gf_false;
 }
 
-int
-xattr_is_equal (dict_t *this, char *key1, data_t *value1, void *data)
+static gf_boolean_t
+afr_xattr_match (dict_t *this, char *key1, data_t *value1, void *data)
 {
-        dict_t *xattr2 = (dict_t *)data;
-        data_t *value2 = NULL;
-
-        if (afr_is_xattr_ignorable (key1))
-                return 0;
-
-        value2 = dict_get (xattr2, key1);
-        if (!value2)
-                return -1;
-
-        if (value1->len != value2->len)
-                return -1;
-        if(memcmp(value1->data, value2->data, value1->len))
-                return -1;
-        else
-                return 0;
+        if (!afr_is_xattr_ignorable (key1))
+                return _gf_true;
 
+        return _gf_false;
 }
 
-/* To conclude that both dicts are equal, we need to check if
- * 1) For every key-val pair in dict1, a match is present in dict2
- * 2) For every key-val pair in dict2, a match is present in dict1
- * We need to do both because ignoring glusterfs' internal xattrs
- * happens only in xattr_is_equal().
- */
 gf_boolean_t
 afr_xattrs_are_equal (dict_t *dict1, dict_t *dict2)
 {
-        int ret = 0;
-
-        ret = dict_foreach (dict1, xattr_is_equal, dict2);
-        if (ret == -1)
-                return _gf_false;
-
-        ret = dict_foreach (dict2, xattr_is_equal, dict1);
-        if (ret == -1)
-                 return _gf_false;
-
-        return _gf_true;
+        return are_dicts_equal (dict1, dict2, afr_xattr_match, NULL);
 }
 
 static int
diff --git a/xlators/cluster/ec/src/ec-combine.c b/xlators/cluster/ec/src/ec-combine.c
index e84055c51e4..9d4a18999f1 100644
--- a/xlators/cluster/ec/src/ec-combine.c
+++ b/xlators/cluster/ec/src/ec-combine.c
@@ -168,96 +168,40 @@ void ec_iatt_rebuild(ec_t * ec, struct iatt * iatt, int32_t count,
     }
 }
 
-int32_t ec_dict_data_compare(dict_t * dict, char * key, data_t * value,
-                             void * arg)
+gf_boolean_t
+ec_xattr_match (dict_t *dict, char *key, data_t *value, void *arg)
 {
-    ec_dict_info_t * info = arg;
-    data_t * data;
-
-    data = dict_get(info->dict, key);
-    if (data == NULL)
-    {
-        gf_log("ec", GF_LOG_DEBUG, "key '%s' found only on one dict", key);
-
-        return -1;
-    }
-
-    info->count--;
-
-    if ((strcmp(key, GF_CONTENT_KEY) == 0) ||
-        (strcmp(key, GF_XATTR_PATHINFO_KEY) == 0) ||
-        (strcmp(key, GF_XATTR_USER_PATHINFO_KEY) == 0) ||
-        (strcmp(key, GF_XATTR_LOCKINFO_KEY) == 0) ||
-        (strcmp(key, GLUSTERFS_OPEN_FD_COUNT) == 0) ||
-        (strncmp(key, GF_XATTR_CLRLK_CMD, strlen(GF_XATTR_CLRLK_CMD)) == 0) ||
-        (strncmp(key, EC_QUOTA_PREFIX, strlen(EC_QUOTA_PREFIX)) == 0) ||
-        (fnmatch(GF_XATTR_STIME_PATTERN, key, 0) == 0) ||
-        (fnmatch(MARKER_XATTR_PREFIX ".*." XTIME, key, 0) == 0) ||
-        (fnmatch(GF_XATTR_MARKER_KEY ".*", key, 0) == 0) ||
-        (XATTR_IS_NODE_UUID(key)))
-    {
-        return 0;
-    }
+        if (fnmatch(GF_XATTR_STIME_PATTERN, key, 0) == 0)
+                return _gf_false;
 
-    if ((data->len != value->len) ||
-        (memcmp(data->data, value->data, data->len) != 0))
-    {
-        gf_log("ec", GF_LOG_DEBUG, "key '%s' is different (size: %u, %u)",
-               key, data->len, value->len);
-
-        return -1;
-    }
-
-    return 0;
+        return _gf_true;
 }
 
-int32_t ec_dict_data_show(dict_t * dict, char * key, data_t * value,
-                          void * arg)
+gf_boolean_t
+ec_value_ignore (char *key)
 {
-    if (dict_get(arg, key) == NULL)
-    {
-        gf_log("ec", GF_LOG_DEBUG, "key '%s' found only on one dict", key);
-    }
-
-    return 0;
+        if ((strcmp(key, GF_CONTENT_KEY) == 0) ||
+            (strcmp(key, GF_XATTR_PATHINFO_KEY) == 0) ||
+            (strcmp(key, GF_XATTR_USER_PATHINFO_KEY) == 0) ||
+            (strcmp(key, GF_XATTR_LOCKINFO_KEY) == 0) ||
+            (strcmp(key, GLUSTERFS_OPEN_FD_COUNT) == 0) ||
+            (strncmp(key, GF_XATTR_CLRLK_CMD,
+                     strlen (GF_XATTR_CLRLK_CMD)) == 0) ||
+            (strncmp(key, EC_QUOTA_PREFIX, strlen(EC_QUOTA_PREFIX)) == 0) ||
+            (fnmatch(MARKER_XATTR_PREFIX ".*." XTIME, key, 0) == 0) ||
+            (fnmatch(GF_XATTR_MARKER_KEY ".*", key, 0) == 0) ||
+            (XATTR_IS_NODE_UUID(key))) {
+                return _gf_true;
+        }
+        return _gf_false;
 }
 
-int32_t ec_dict_compare(dict_t * dict1, dict_t * dict2)
+int32_t
+ec_dict_compare (dict_t *dict1, dict_t *dict2)
 {
-    ec_dict_info_t info;
-    dict_t * dict;
-
-    if (dict1 != NULL)
-    {
-        info.dict = dict1;
-        info.count = dict1->count;
-        dict = dict2;
-    }
-    else if (dict2 != NULL)
-    {
-        info.dict = dict2;
-        info.count = dict2->count;
-        dict = dict1;
-    }
-    else
-    {
-        return 1;
-    }
-
-    if (dict != NULL)
-    {
-        if (dict_foreach(dict, ec_dict_data_compare, &info) != 0)
-        {
-            return 0;
-        }
-    }
-
-    if (info.count != 0)
-    {
-        dict_foreach(info.dict, ec_dict_data_show, dict);
-    }
-
-    return (info.count == 0);
+        if (are_dicts_equal (dict1, dict2, ec_xattr_match, ec_value_ignore))
+                return 1;
+        return 0;
 }
 
 int32_t ec_dict_list(data_t ** list, int32_t * count, ec_cbk_data_t * cbk,
diff --git a/xlators/cluster/ec/src/ec-inode-read.c b/xlators/cluster/ec/src/ec-inode-read.c
index 365ea3db0ec..f87df4016c0 100644
--- a/xlators/cluster/ec/src/ec-inode-read.c
+++ b/xlators/cluster/ec/src/ec-inode-read.c
@@ -234,6 +234,25 @@ void ec_wind_getxattr(ec_t * ec, ec_fop_data_t * fop, int32_t idx)
                       &fop->loc[0], fop->str[0], fop->xdata);
 }
 
+void
+ec_handle_special_xattrs (ec_fop_data_t *fop)
+{
+        ec_cbk_data_t *cbk = NULL;
+        /* Stime may not be available on all the bricks, so even if some of the
+         * subvols succeed the operation, treat it as answer.*/
+        if (fop->str[0] &&
+            fnmatch (GF_XATTR_STIME_PATTERN, fop->str[0], 0) == 0) {
+                if (!fop->answer || (fop->answer->op_ret < 0)) {
+                        list_for_each_entry (cbk, &fop->cbk_list, list) {
+                                if (cbk->op_ret >= 0) {
+                                        fop->answer = cbk;
+                                        break;
+                                }
+                        }
+                }
+        }
+}
+
 int32_t ec_manager_getxattr(ec_fop_data_t * fop, int32_t state)
 {
     ec_cbk_data_t * cbk;
@@ -263,6 +282,7 @@ int32_t ec_manager_getxattr(ec_fop_data_t * fop, int32_t state)
             return EC_STATE_PREPARE_ANSWER;
 
         case EC_STATE_PREPARE_ANSWER:
+            ec_handle_special_xattrs (fop);
             cbk = fop->answer;
             if (cbk != NULL)
             {
diff --git a/xlators/cluster/ec/src/ec.c b/xlators/cluster/ec/src/ec.c
index 5476ac6562e..3dd04299541 100644
--- a/xlators/cluster/ec/src/ec.c
+++ b/xlators/cluster/ec/src/ec.c
@@ -670,6 +670,7 @@ ec_gf_getxattr (call_frame_t *frame, xlator_t *this, loc_t *loc,
 {
         int     error = 0;
         ec_t    *ec = this->private;
+        int32_t minimum = EC_MINIMUM_MIN;
 
         if (name && strcmp (name, EC_XATTR_HEAL) != 0) {
                 EC_INTERNAL_XATTR_OR_GOTO(name, NULL, error, out);
@@ -682,7 +683,10 @@ ec_gf_getxattr (call_frame_t *frame, xlator_t *this, loc_t *loc,
                                             NULL, ec_marker_populate_args) == 0)
                 return 0;
 
-        ec_getxattr (frame, this, -1, EC_MINIMUM_MIN, default_getxattr_cbk,
+        if (name && (fnmatch (GF_XATTR_STIME_PATTERN, name, 0) == 0))
+                minimum = EC_MINIMUM_ALL;
+
+        ec_getxattr (frame, this, -1, minimum, default_getxattr_cbk,
                      NULL, loc, name, xdata);
 
         return 0;