snapshot/cli: Fix snapshot status xml output

snap status --xml errors out if a brick is down and
doesn't have pid. It is handled in the cli of the snap
status where "N/A" is displayed in such a scenario.
Handled the same in xml

snap status <snapname> --xml fails as the writer is
not initialised for the same. Using GF_SNAP_STATUS_TYPE_ITER
instead of GF_SNAP_STATUS_TYPE_SNAP for all snap's
status to differentiate between the two scenarios.

Added testcase volume-snapshot-xml.t to check
all snapshot commands xml outputs

Change-Id: I99563e8f3e84f1aaeabd865326bb825c44f5c745
BUG: 1325831
Signed-off-by: Avra Sengupta <asengupt@redhat.com>
Reviewed-on: http://review.gluster.org/14018
Smoke: Gluster Build System <jenkins@build.gluster.org>
CentOS-regression: Gluster Build System <jenkins@build.gluster.org>
NetBSD-regression: NetBSD Build System <jenkins@build.gluster.org>
Reviewed-by: Rajesh Joseph <rjoseph@redhat.com>

5 files changed, 136 insertions, 38 deletions

diff --git a/cli/src/cli-rpc-ops.c b/cli/src/cli-rpc-ops.c
index 0bf03bceaa2..441f4247c45 100644
--- a/cli/src/cli-rpc-ops.c
+++ b/cli/src/cli-rpc-ops.c
@@ -9908,7 +9908,7 @@ cli_populate_req_dict_for_status (dict_t *snap_dict, dict_t *dict, int index) {
         GF_ASSERT (dict);
 
         ret = dict_set_uint32 (snap_dict, "sub-cmd",
-                               GF_SNAP_STATUS_TYPE_SNAP);
+                               GF_SNAP_STATUS_TYPE_ITER);
         if (ret) {
                 gf_log ("cli", GF_LOG_ERROR, "Could not save command "
                         "type in snap dict");
@@ -10000,7 +10000,8 @@ cli_snapshot_status (dict_t *dict, gf_cli_rsp *rsp,
                 goto out;
         }
 
-        if (status_cmd != GF_SNAP_STATUS_TYPE_SNAP) {
+        if ((status_cmd != GF_SNAP_STATUS_TYPE_SNAP) &&
+            (status_cmd != GF_SNAP_STATUS_TYPE_ITER)) {
                 dict_copy (dict, local->dict);
                 goto out;
         }
@@ -10484,7 +10485,8 @@ gf_cli_snapshot_for_status (call_frame_t *frame, xlator_t *this,
          * is already handled. Therefore we can return from here.
          * If want to get status of all snaps in the system or volume then
          * we should get them one by one.*/
-        if (cmd == GF_SNAP_STATUS_TYPE_SNAP) {
+        if ((cmd == GF_SNAP_STATUS_TYPE_SNAP) ||
+            (cmd == GF_SNAP_STATUS_TYPE_ITER)) {
                 ret = 0;
                 goto out;
         }
diff --git a/cli/src/cli-xml-output.c b/cli/src/cli-xml-output.c
index 35b739c0f62..f6158eab794 100644
--- a/cli/src/cli-xml-output.c
+++ b/cli/src/cli-xml-output.c
@@ -5146,8 +5146,16 @@ cli_xml_snapshot_volume_status (xmlTextWriterPtr writer, xmlDocPtr doc,
 
                 ret = dict_get_str (dict, key, &buffer);
                 if (ret) {
-                        gf_log ("cli", GF_LOG_INFO, "Unable to get Brick Path");
-                        goto out;
+                        gf_log ("cli", GF_LOG_ERROR,
+                                "Unable to get Brick Path");
+                        /*
+                         * If path itself is not present, then end *
+                         * this brick's status and continue to the *
+                         * brick                                   *
+                         */
+                        ret = xmlTextWriterEndElement (writer);
+                        XML_RET_CHECK_AND_GOTO (ret, out);
+                        continue;
                 }
 
                 ret = xmlTextWriterWriteFormatElement (writer,
@@ -5159,14 +5167,14 @@ cli_xml_snapshot_volume_status (xmlTextWriterPtr writer, xmlDocPtr doc,
 
                 ret = dict_get_str (dict, key, &buffer);
                 if (ret) {
-                        gf_log ("cli", GF_LOG_INFO,
+                        gf_log ("cli", GF_LOG_ERROR,
                                 "Unable to get Volume Group");
-                        goto out;
-                }
+                        ret = xmlTextWriterWriteFormatElement (writer,
+                                              (xmlChar *) "volumeGroup", "N/A");
+                } else
+                        ret = xmlTextWriterWriteFormatElement (writer,
+                                       (xmlChar *) "volumeGroup", "%s", buffer);
 
-                ret = xmlTextWriterWriteFormatElement (writer,
-                                        (xmlChar *) "volumeGroup",
-                                        "%s", buffer);
                 XML_RET_CHECK_AND_GOTO (ret, out);
 
                 snprintf (key, sizeof (key), "%s.brick%d.status", keyprefix, i);
@@ -5175,22 +5183,25 @@ cli_xml_snapshot_volume_status (xmlTextWriterPtr writer, xmlDocPtr doc,
                 if (ret) {
                         gf_log ("cli", GF_LOG_INFO,
                                 "Unable to get Brick Running");
-                        strcpy (buffer, "N/A");
-                }
-                ret = xmlTextWriterWriteFormatElement (writer,
-                                    (xmlChar *) "BrickRunning", "%s", buffer);
+                        ret = xmlTextWriterWriteFormatElement (writer,
+                                            (xmlChar *) "brick_running", "N/A");
+                } else
+                        ret = xmlTextWriterWriteFormatElement (writer,
+                                     (xmlChar *) "brick_running", "%s", buffer);
+
                 XML_RET_CHECK_AND_GOTO (ret, out);
 
                 snprintf (key, sizeof (key), "%s.brick%d.pid", keyprefix, i);
 
                 ret = dict_get_int32 (dict, key, &pid);
                 if (ret) {
-                        gf_log ("cli", GF_LOG_ERROR, "Unable to get pid");
-                        goto out;
-                }
+                        gf_log ("cli", GF_LOG_INFO, "Unable to get pid");
+                        ret = xmlTextWriterWriteFormatElement (writer,
+                                                      (xmlChar *) "pid", "N/A");
+                } else
+                        ret = xmlTextWriterWriteFormatElement (writer,
+                                                  (xmlChar *) "pid", "%d", pid);
 
-                ret = xmlTextWriterWriteFormatElement (writer,
-                                        (xmlChar *) "pid", "%d", pid);
                 XML_RET_CHECK_AND_GOTO (ret, out);
 
                 snprintf (key, sizeof (key), "%s.brick%d.data", keyprefix, i);
@@ -5199,28 +5210,29 @@ cli_xml_snapshot_volume_status (xmlTextWriterPtr writer, xmlDocPtr doc,
                 if (ret) {
                         gf_log ("cli", GF_LOG_ERROR,
                                         "Unable to get Data Percent");
-                        goto out;
-                }
+                        ret = xmlTextWriterWriteFormatElement (writer,
+                                          (xmlChar *) "data_percentage", "N/A");
+                } else
+                        ret = xmlTextWriterWriteFormatElement (writer,
+                                   (xmlChar *) "data_percentage", "%s", buffer);
 
-                ret = xmlTextWriterWriteFormatElement (writer,
-                                                       (xmlChar *) "lvUsage",
-                                                       "%s", buffer);
                 XML_RET_CHECK_AND_GOTO (ret, out);
 
                 snprintf (key, sizeof (key), "%s.brick%d.lvsize",
                           keyprefix, i);
                 ret = dict_get_str (dict, key, &buffer);
                 if (ret) {
-                        gf_log ("cli", GF_LOG_INFO, "Unable to get LV Size");
-                        goto out;
-                }
+                        gf_log ("cli", GF_LOG_ERROR, "Unable to get LV Size");
+                        ret = xmlTextWriterWriteFormatElement (writer,
+                                                 (xmlChar *) "lvSize", "N/A");
+                } else {
+                        /* Truncate any newline character */
+                        buffer = strtok (buffer, "\n");
 
-                /* Truncate any newline character */
-                buffer = strtok (buffer, "\n");
+                        ret = xmlTextWriterWriteFormatElement (writer,
+                                          (xmlChar *) "lvSize", "%s", buffer);
+                }
 
-                ret = xmlTextWriterWriteFormatElement (writer,
-                                                       (xmlChar *) "lvSize",
-                                                       "%s", buffer);
                 XML_RET_CHECK_AND_GOTO (ret, out);
 
                 /* </brick> */
@@ -5358,7 +5370,8 @@ cli_xml_snapshot_status (xmlTextWriterPtr writer, xmlDocPtr doc, dict_t *dict)
                 goto out;
         }
 
-        if (GF_SNAP_STATUS_TYPE_SNAP == status_cmd) {
+        if ((GF_SNAP_STATUS_TYPE_SNAP == status_cmd) ||
+            (GF_SNAP_STATUS_TYPE_ITER == status_cmd)) {
                 snapcount = 1;
         } else {
                 ret = dict_get_int32 (dict, "status.snapcount", &snapcount);
@@ -6123,7 +6136,7 @@ cli_xml_snapshot_begin_composite_op (cli_local_t *local)
                 goto out;
         }
 
-        if (cmd == GF_SNAP_STATUS_TYPE_SNAP ||
+        if (cmd == GF_SNAP_STATUS_TYPE_ITER ||
             cmd == GF_SNAP_DELETE_TYPE_SNAP){
                 ret = 0;
                 goto out;
@@ -6168,7 +6181,7 @@ cli_xml_snapshot_end_composite_op (cli_local_t *local)
                 goto out;
         }
 
-        if (cmd == GF_SNAP_STATUS_TYPE_SNAP ||
+        if (cmd == GF_SNAP_STATUS_TYPE_ITER ||
             cmd == GF_SNAP_DELETE_TYPE_SNAP){
                 ret = 0;
                 goto out;
diff --git a/rpc/xdr/src/cli1-xdr.x b/rpc/xdr/src/cli1-xdr.x
index f1d4ea93d6d..66d399bfaf8 100644
--- a/rpc/xdr/src/cli1-xdr.x
+++ b/rpc/xdr/src/cli1-xdr.x
@@ -197,10 +197,15 @@ enum  gf1_cli_snapshot_status {
         GF_SNAP_STATUS_TYPE_ITER
 };
 
+/* Changing order of GF_SNAP_DELETE_TYPE_VOL           *
+ * and GF_SNAP_DELETE_TYPE_SNAP so that they don't     *
+ * overlap with the enums of GF_SNAP_STATUS_TYPE_SNAP, *
+ * and GF_SNAP_STATUS_TYPE_VOL                         *
+ */
 enum gf1_cli_snapshot_delete {
         GF_SNAP_DELETE_TYPE_ALL  = 0,
-        GF_SNAP_DELETE_TYPE_SNAP = 1,
-        GF_SNAP_DELETE_TYPE_VOL  = 2,
+        GF_SNAP_DELETE_TYPE_VOL  = 1,
+        GF_SNAP_DELETE_TYPE_SNAP = 2,
         GF_SNAP_DELETE_TYPE_ITER = 3
 };
 
diff --git a/tests/basic/volume-snapshot-xml.t b/tests/basic/volume-snapshot-xml.t
new file mode 100755
index 00000000000..d58e898083a
--- /dev/null
+++ b/tests/basic/volume-snapshot-xml.t
@@ -0,0 +1,76 @@
+#!/bin/bash
+
+. $(dirname $0)/../include.rc
+. $(dirname $0)/../snapshot.rc
+
+function get-xml()
+{
+        $CLI $1 --xml | xmllint --format - | grep $2 | sed 's/\(<"$2">\|<\/"$2">\)//g'
+}
+
+cleanup;
+TEST verify_lvm_version;
+TEST glusterd;
+TEST pidof glusterd;
+
+TEST setup_lvm 1
+
+TEST $CLI volume create $V0 $H0:$L1
+TEST $CLI volume start $V0
+
+# Snapshot config xmls
+EXPECT "enable" get-xml "snapshot config activate-on-create enable" "activateOnCreate"
+EXPECT "100" get-xml "snapshot config $V0 snap-max-hard-limit 100" "newHardLimit"
+EXPECT "70" get-xml "snapshot config snap-max-soft-limit 70" "newSoftLimit"
+EXPECT "enable" get-xml "snapshot config auto-delete enable" "autoDelete"
+
+# Snapshot create, activate, deactivate xmls
+EXPECT "snap1" get-xml "snapshot create snap1 $V0 no-timestamp" "name"
+EXPECT "snap1" get-xml "snapshot deactivate snap1" "name"
+EXPECT "snap1" get-xml "snapshot activate snap1" "name"
+EXPECT "snap2" get-xml "snapshot create snap2 $V0 no-timestamp" "name"
+
+# Snapshot info xmls
+EXPECT "2" get-xml "snapshot info" "count"
+EXPECT "Started" get-xml "snapshot info" "status"
+EXPECT "2" get-xml "snapshot info volume $V0" "count"
+EXPECT "Started" get-xml "snapshot info volume $V0" "status"
+EXPECT "1" get-xml "snapshot info snap1" "count"
+EXPECT "2" get-xml "snapshot info snap1" "snapCount"
+EXPECT "Started" get-xml "snapshot info snap1" "status"
+
+# Snapshot list xmls
+EXPECT "2" get-xml "snapshot list" "count"
+EXPECT "snap2" get-xml "snapshot list $V0" "snapshot"
+
+# Snapshot status xmls
+EXPECT "snap2" get-xml "snapshot status" "name"
+EXPECT "snap2" get-xml "snapshot deactivate snap2" "name"
+EXPECT "N/A" get-xml "snapshot status" "pid"
+EXPECT "snap1" get-xml "snapshot status snap1" "name"
+EXPECT "Yes" get-xml "snapshot status snap1" "brick_running"
+
+# Snapshot restore xmls
+TEST $CLI volume stop $V0
+EXPECT "snap2" get-xml "snapshot restore snap2" "name"
+EXPECT "30807" get-xml "snapshot restore snap2" "opErrno"
+EXPECT "0" get-xml "snapshot restore snap1" "opErrno"
+
+# Snapshot delete xmls
+TEST $CLI volume start $V0
+EXPECT "snap1" get-xml "snapshot create snap1 $V0 no-timestamp" "name"
+EXPECT "snap2" get-xml "snapshot create snap2 $V0 no-timestamp" "name"
+EXPECT "snap3" get-xml "snapshot create snap3 $V0 no-timestamp" "name"
+EXPECT "Success" get-xml "snapshot delete snap3" "status"
+EXPECT "Success" get-xml "snapshot delete all" "status"
+EXPECT "0" get-xml "snapshot list" "count"
+EXPECT "snap1" get-xml "snapshot create snap1 $V0 no-timestamp" "name"
+EXPECT "snap2" get-xml "snapshot create snap2 $V0 no-timestamp" "name"
+EXPECT "snap3" get-xml "snapshot create snap3 $V0 no-timestamp" "name"
+EXPECT "Success" get-xml "snapshot delete volume $V0" "status"
+EXPECT "0" get-xml "snapshot list" "count"
+
+# Snapshot clone xmls
+# Snapshot clone xml is broken. Once it is fixed it will be added here.
+
+cleanup;
diff --git a/xlators/mgmt/glusterd/src/glusterd-snapshot.c b/xlators/mgmt/glusterd/src/glusterd-snapshot.c
index e38395e1eb3..5a9ba003578 100644
--- a/xlators/mgmt/glusterd/src/glusterd-snapshot.c
+++ b/xlators/mgmt/glusterd/src/glusterd-snapshot.c
@@ -5946,6 +5946,7 @@ glusterd_snapshot_status_prevalidate (dict_t *dict, char **op_errstr,
                 {
                         break;
                 }
+                case GF_SNAP_STATUS_TYPE_ITER:
                 case GF_SNAP_STATUS_TYPE_SNAP:
                 {
                         ret = dict_get_str (dict, "snapname", &snapname);
@@ -7865,6 +7866,7 @@ glusterd_snapshot_status_commit (dict_t *dict, char **op_errstr,
                         }
                         break;
                 }
+                case GF_SNAP_STATUS_TYPE_ITER:
                 case GF_SNAP_STATUS_TYPE_SNAP:
                 {