rbd tool changed format? (breaks compatibility)

rbd tool changed format? (breaks compatibility)

[Constantinos Venetsanopoulos]

Hello ceph team,

As you may already know, our team in GRNET is building a complete open
source cloud platform called Synnefo [1], which already powers our
production public cloud service ~okeanos [2].

Synnefo is using Google Ganeti for the low level VM management part [3].
As of Jan 2012, we have merged to upstream Ganeti support for VM disks
on RADOS [4].

Today we received some feedback, that other people trying to run Ganeti
with RADOS get an error because probably the output of the 'rbd
showmapped' command has changed.

I'd like to ask if indeed the output format of the rbd tool has changed.
More specifically:

1. Does the 'rbd showmapped' command still returns just the headers if
no device is mapped?
2. Has the separator between the 'rbd showmapped' columns changed
from \t to ""?

I don't have the latest rbd tool setup (but rather
ceph-common=0.48.1argonaut-1~bpo60+1), so I can't test it right now,
but I see this commit:

https://github.com/ceph/ceph/commit/bed55369a96c2651f513b8c9b1a7bb92fb87550a

How stable can we consider rbd tool's output format?
This is something we want to run in production environment. Using the
tool rather than the library makes things much simpler.


Kind Regards,
Constantinos Venetsanopoulos

[1] http://synnefo.org
[2] http://okeanos.io
[3] http://code.google.com/p/ganeti/
[4] 
http://git.ganeti.org/?p=ganeti.git;a=commit;h=7181fba0662246b5f02764a5b3b0d500f6934dd0

Re: rbd tool changed format? (breaks compatibility)

[Josh Durgin]

On 11/16/2012 06:36 AM, Constantinos Venetsanopoulos wrote:
> Hello ceph team,
>
> As you may already know, our team in GRNET is building a complete open
> source cloud platform called Synnefo [1], which already powers our
> production public cloud service ~okeanos [2].
>
> Synnefo is using Google Ganeti for the low level VM management part [3].
> As of Jan 2012, we have merged to upstream Ganeti support for VM disks
> on RADOS [4].
>
> Today we received some feedback, that other people trying to run Ganeti
> with RADOS get an error because probably the output of the 'rbd
> showmapped' command has changed.
>
> I'd like to ask if indeed the output format of the rbd tool has changed.
> More specifically:
>
> 1. Does the 'rbd showmapped' command still returns just the headers if
> no device is mapped?

No

> 2. Has the separator between the 'rbd showmapped' columns changed
> from \t to ""?

Yes, this is in the release notes for 0.54 
(http://ceph.com/docs/master/release-notes/#v0-54).

> I don't have the latest rbd tool setup (but rather
> ceph-common=0.48.1argonaut-1~bpo60+1), so I can't test it right now,
> but I see this commit:
>
> https://github.com/ceph/ceph/commit/bed55369a96c2651f513b8c9b1a7bb92fb87550a

Yeah, that's the commit that changed it.

> How stable can we consider rbd tool's output format?
> This is something we want to run in production environment. Using the
> tool rather than the library makes things much simpler.

Generally it won't change much, but I don't think it should be
considered entirely unchanging. We'll add it to the release notes when
the output does change. We'll probably switch other commands to use
TextTable too, with the same results as with showmapped and lock list.
We could send a message to the mailing list when the output changes as
well, so you can prepare for a future release.

Perhaps we should add a --format json|plain option so you don't have to
rely on particular formatting, you just parse the json. This would
match existing usage by many 'ceph ...' commands, and be easier
for scripts to use in general.

Josh

Re: rbd tool changed format? (breaks compatibility)

[Constantinos Venetsanopoulos]

On 11/16/2012 07:14 PM, Josh Durgin wrote:
> On 11/16/2012 06:36 AM, Constantinos Venetsanopoulos wrote:
>> Hello ceph team,
>>
>> As you may already know, our team in GRNET is building a complete open
>> source cloud platform called Synnefo [1], which already powers our
>> production public cloud service ~okeanos [2].
>>
>> Synnefo is using Google Ganeti for the low level VM management part [3].
>> As of Jan 2012, we have merged to upstream Ganeti support for VM disks
>> on RADOS [4].
>>
>> Today we received some feedback, that other people trying to run Ganeti
>> with RADOS get an error because probably the output of the 'rbd
>> showmapped' command has changed.
>>
>> I'd like to ask if indeed the output format of the rbd tool has changed.
>> More specifically:
>>
>> 1. Does the 'rbd showmapped' command still returns just the headers if
>> no device is mapped?
>
> No

Ack.

>
>
>> 2. Has the separator between the 'rbd showmapped' columns changed
>> from \t to ""?
>
> Yes, this is in the release notes for 0.54 
> (http://ceph.com/docs/master/release-notes/#v0-54).
>

Ack.

>> I don't have the latest rbd tool setup (but rather
>> ceph-common=0.48.1argonaut-1~bpo60+1), so I can't test it right now,
>> but I see this commit:
>>
>> https://github.com/ceph/ceph/commit/bed55369a96c2651f513b8c9b1a7bb92fb87550a 
>>
>
> Yeah, that's the commit that changed it.
>
>> How stable can we consider rbd tool's output format?
>> This is something we want to run in production environment. Using the
>> tool rather than the library makes things much simpler.
>
> Generally it won't change much, but I don't think it should be
> considered entirely unchanging. We'll add it to the release notes when
> the output does change. We'll probably switch other commands to use
> TextTable too, with the same results as with showmapped and lock list.
> We could send a message to the mailing list when the output changes as
> well, so you can prepare for a future release.

That would be great, and highly appreciated. Please drop us an email at
the following mailing lists, when the rbd tool's format changes:

synnefo-devel@googlegroups.com
ganeti-devel@googlegroups.com

>
> Perhaps we should add a --format json|plain option so you don't have to
> rely on particular formatting, you just parse the json. This would
> match existing usage by many 'ceph ...' commands, and be easier
> for scripts to use in general.

That would be even better! That would be the best approach for us, since
we use it inside python code. Parsing a json is very simple and we will be
able to maintain compatibility even when the format changes.

Thanks,
Consantinos

[PATCH] rbd: Add --json flag for the showmapped command

[Stratos Psomadakis]

Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
---
Hi Josh,

This patch adds the '--json' flag to enable dumping the showmapped output in
json format (as you suggested). I'm not sure if any other rbd subcommands could
make use of this flag (so the patch is showmapped-specific atm).

Comments?

Thanks,
Stratos

 man/rbd.8  |    5 +++++
 src/rbd.cc |   54 +++++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/man/rbd.8 b/man/rbd.8
index 6004244..7bf4c39 100644
--- a/man/rbd.8
+++ b/man/rbd.8
@@ -132,6 +132,11 @@ be open from more than one client at once, like during
 live migration of a virtual machine, or for use underneath
 a clustered filesystem.
 .UNINDENT
+.INDENT 0.0
+.TP
+.B \-\-json
+Dump output in json format (currently used only by showmapped).
+.UNINDENT
 .SH COMMANDS
 .INDENT 0.0
 .TP
diff --git a/src/rbd.cc b/src/rbd.cc
index 3db8978..82a72ba 100644
--- a/src/rbd.cc
+++ b/src/rbd.cc
@@ -47,6 +47,8 @@
 #include "include/rbd_types.h"
 #include "common/TextTable.h"
 
+#include "common/Formatter.h"
+
 #if defined(__linux__)
 #include <linux/fs.h>
 #endif
@@ -126,7 +128,8 @@ void usage()
 "                               format 2 supports cloning\n"
 "  --id <username>              rados user (without 'client.' prefix) to authenticate as\n"
 "  --keyfile <path>             file containing secret key for use with cephx\n"
-"  --shared <tag>               take a shared (rather than exclusive) lock\n";
+"  --shared <tag>               take a shared (rather than exclusive) lock\n"
+"  --json                       dump output in json format (currently used only by showmapped)\n";
 }
 
 static string feature_str(uint64_t features)
@@ -1198,7 +1201,7 @@ void do_closedir(DIR *dp)
     closedir(dp);
 }
 
-static int do_kernel_showmapped()
+static int do_kernel_showmapped(bool dump_json)
 {
   int r;
   bool have_output = false;
@@ -1220,12 +1223,18 @@ static int do_kernel_showmapped()
     return r;
   }
 
+  JSONFormatter jsf(false);
   TextTable tbl;
-  tbl.define_column("id", TextTable::LEFT, TextTable::LEFT);
-  tbl.define_column("pool", TextTable::LEFT, TextTable::LEFT);
-  tbl.define_column("image", TextTable::LEFT, TextTable::LEFT);
-  tbl.define_column("snap", TextTable::LEFT, TextTable::LEFT);
-  tbl.define_column("device", TextTable::LEFT, TextTable::LEFT);
+  if(dump_json == false) {
+    tbl.define_column("id", TextTable::LEFT, TextTable::LEFT);
+    tbl.define_column("pool", TextTable::LEFT, TextTable::LEFT);
+    tbl.define_column("image", TextTable::LEFT, TextTable::LEFT);
+    tbl.define_column("snap", TextTable::LEFT, TextTable::LEFT);
+    tbl.define_column("device", TextTable::LEFT, TextTable::LEFT);
+  }
+  else {
+    jsf.open_object_section("devices");
+  }
 
   do {
     if (strcmp(dent->d_name, ".") == 0 || strcmp(dent->d_name, "..") == 0)
@@ -1262,13 +1271,30 @@ static int do_kernel_showmapped()
 	   << cpp_strerror(-r) << std::endl;
       continue;
     }
-
-    tbl << dent->d_name << pool << name << snap << dev << TextTable::endrow;
+    
+    if (dump_json == false) {
+      tbl << dent->d_name << pool << name << snap << dev << TextTable::endrow;
+    }
+    else {
+      jsf.open_object_section(dent->d_name);
+      jsf.dump_string("pool", pool);
+      jsf.dump_string("name", name);
+      jsf.dump_string("snap", snap);
+      jsf.dump_string("device", dev);
+      jsf.close_section();
+    }
     have_output = true;
 
   } while ((dent = readdir(device_dir.get())));
-  if (have_output)
-    cout << tbl;
+
+  if (dump_json == false) {
+    if (have_output)
+      cout << tbl;
+  }
+  else {
+    jsf.close_section();
+    jsf.flush(cout);
+  }
 
   return 0;
 }
@@ -1505,7 +1531,7 @@ int main(int argc, const char **argv)
   const char *poolname = NULL;
   uint64_t size = 0;  // in bytes
   int order = 0;
-  bool format_specified = false;
+  bool format_specified = false, dump_json = false;
   int format = 1;
   uint64_t features = RBD_FEATURE_LAYERING;
   const char *imgname = NULL, *snapname = NULL, *destname = NULL,
@@ -1579,6 +1605,8 @@ int main(int argc, const char **argv)
       imgname = strdup(val.c_str());
     } else if (ceph_argparse_witharg(args, i, &val, "--shared", (char *)NULL)) {
       lock_tag = strdup(val.c_str());
+    } else if (ceph_argparse_flag(args, i, "--json", (char *) NULL)) {
+      dump_json = true;
     } else {
       ++i;
     }
@@ -2130,7 +2158,7 @@ if (!set_conf_param(v, p1, p2, p3)) { \
     break;
 
   case OPT_SHOWMAPPED:
-    r = do_kernel_showmapped();
+    r = do_kernel_showmapped(dump_json);
     if (r < 0) {
       cerr << "rbd: showmapped failed: " << cpp_strerror(-r) << std::endl;
       return EXIT_FAILURE;
-- 
1.7.10.4

Re: [PATCH] rbd: Add --json flag for the showmapped command

[Yehuda Sadeh]

On Thu, Dec 13, 2012 at 7:37 AM, Stratos Psomadakis <psomas@grnet.gr> wrote:
> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
> ---
> Hi Josh,
>
> This patch adds the '--json' flag to enable dumping the showmapped output in

I think that it should be "--format=json" rather than --json. This
will make it more in line with other tools. Also, then you could have
the unpopular --format=xml.

> json format (as you suggested). I'm not sure if any other rbd subcommands could
> make use of this flag (so the patch is showmapped-specific atm).
>
> Comments?
>
> Thanks,
> Stratos
>
>  man/rbd.8  |    5 +++++
>  src/rbd.cc |   54 +++++++++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 46 insertions(+), 13 deletions(-)
>
> diff --git a/man/rbd.8 b/man/rbd.8
> index 6004244..7bf4c39 100644
> --- a/man/rbd.8
> +++ b/man/rbd.8
> @@ -132,6 +132,11 @@ be open from more than one client at once, like during
>  live migration of a virtual machine, or for use underneath
>  a clustered filesystem.
>  .UNINDENT
> +.INDENT 0.0
> +.TP
> +.B \-\-json
> +Dump output in json format (currently used only by showmapped).
> +.UNINDENT
>  .SH COMMANDS
>  .INDENT 0.0
>  .TP
> diff --git a/src/rbd.cc b/src/rbd.cc
> index 3db8978..82a72ba 100644
> --- a/src/rbd.cc
> +++ b/src/rbd.cc
> @@ -47,6 +47,8 @@
>  #include "include/rbd_types.h"
>  #include "common/TextTable.h"
>
> +#include "common/Formatter.h"
> +
>  #if defined(__linux__)
>  #include <linux/fs.h>
>  #endif
> @@ -126,7 +128,8 @@ void usage()
>  "                               format 2 supports cloning\n"
>  "  --id <username>              rados user (without 'client.' prefix) to authenticate as\n"
>  "  --keyfile <path>             file containing secret key for use with cephx\n"
> -"  --shared <tag>               take a shared (rather than exclusive) lock\n";
> +"  --shared <tag>               take a shared (rather than exclusive) lock\n"
> +"  --json                       dump output in json format (currently used only by showmapped)\n";
>  }
>
>  static string feature_str(uint64_t features)
> @@ -1198,7 +1201,7 @@ void do_closedir(DIR *dp)
>      closedir(dp);
>  }
>
> -static int do_kernel_showmapped()
> +static int do_kernel_showmapped(bool dump_json)
>  {
>    int r;
>    bool have_output = false;
> @@ -1220,12 +1223,18 @@ static int do_kernel_showmapped()
>      return r;
>    }
>
> +  JSONFormatter jsf(false);
>    TextTable tbl;
> -  tbl.define_column("id", TextTable::LEFT, TextTable::LEFT);
> -  tbl.define_column("pool", TextTable::LEFT, TextTable::LEFT);
> -  tbl.define_column("image", TextTable::LEFT, TextTable::LEFT);
> -  tbl.define_column("snap", TextTable::LEFT, TextTable::LEFT);
> -  tbl.define_column("device", TextTable::LEFT, TextTable::LEFT);
> +  if(dump_json == false) {
> +    tbl.define_column("id", TextTable::LEFT, TextTable::LEFT);
> +    tbl.define_column("pool", TextTable::LEFT, TextTable::LEFT);
> +    tbl.define_column("image", TextTable::LEFT, TextTable::LEFT);
> +    tbl.define_column("snap", TextTable::LEFT, TextTable::LEFT);
> +    tbl.define_column("device", TextTable::LEFT, TextTable::LEFT);
> +  }
> +  else {
> +    jsf.open_object_section("devices");
> +  }
>
>    do {
>      if (strcmp(dent->d_name, ".") == 0 || strcmp(dent->d_name, "..") == 0)
> @@ -1262,13 +1271,30 @@ static int do_kernel_showmapped()
>            << cpp_strerror(-r) << std::endl;
>        continue;
>      }
> -
> -    tbl << dent->d_name << pool << name << snap << dev << TextTable::endrow;
> +
> +    if (dump_json == false) {
> +      tbl << dent->d_name << pool << name << snap << dev << TextTable::endrow;
> +    }
> +    else {
> +      jsf.open_object_section(dent->d_name);
> +      jsf.dump_string("pool", pool);
> +      jsf.dump_string("name", name);
> +      jsf.dump_string("snap", snap);
> +      jsf.dump_string("device", dev);
> +      jsf.close_section();
> +    }
>      have_output = true;
>
>    } while ((dent = readdir(device_dir.get())));
> -  if (have_output)
> -    cout << tbl;
> +
> +  if (dump_json == false) {
> +    if (have_output)
> +      cout << tbl;
> +  }
> +  else {
> +    jsf.close_section();
> +    jsf.flush(cout);
> +  }
>
>    return 0;
>  }
> @@ -1505,7 +1531,7 @@ int main(int argc, const char **argv)
>    const char *poolname = NULL;
>    uint64_t size = 0;  // in bytes
>    int order = 0;
> -  bool format_specified = false;
> +  bool format_specified = false, dump_json = false;
>    int format = 1;
>    uint64_t features = RBD_FEATURE_LAYERING;
>    const char *imgname = NULL, *snapname = NULL, *destname = NULL,
> @@ -1579,6 +1605,8 @@ int main(int argc, const char **argv)
>        imgname = strdup(val.c_str());
>      } else if (ceph_argparse_witharg(args, i, &val, "--shared", (char *)NULL)) {
>        lock_tag = strdup(val.c_str());
> +    } else if (ceph_argparse_flag(args, i, "--json", (char *) NULL)) {
> +      dump_json = true;
>      } else {
>        ++i;
>      }
> @@ -2130,7 +2158,7 @@ if (!set_conf_param(v, p1, p2, p3)) { \
>      break;
>
>    case OPT_SHOWMAPPED:
> -    r = do_kernel_showmapped();
> +    r = do_kernel_showmapped(dump_json);
>      if (r < 0) {
>        cerr << "rbd: showmapped failed: " << cpp_strerror(-r) << std::endl;
>        return EXIT_FAILURE;
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rbd: Add --json flag for the showmapped command

[Stratos Psomadakis]

[-- Attachment #1: Type: text/plain, Size: 6248 bytes --]

On 12/13/2012 07:17 PM, Yehuda Sadeh wrote:
> On Thu, Dec 13, 2012 at 7:37 AM, Stratos Psomadakis <psomas@grnet.gr> wrote:
>> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
>> ---
>> Hi Josh,
>>
>> This patch adds the '--json' flag to enable dumping the showmapped output in
> I think that it should be "--format=json" rather than --json. This
> will make it more in line with other tools. Also, then you could have
> the unpopular --format=xml.

Hi,

the problem is that --format is arleady used in rbd (to select the rbd
image format). So, you could either use --output-format= for
plain/json/xml output (but then you would be inconsistent wrt the other
tools), or change --format to --image-format (or something like that),
and use --format to select the output formatting.

>
>> json format (as you suggested). I'm not sure if any other rbd subcommands could
>> make use of this flag (so the patch is showmapped-specific atm).
>>
>> Comments?
>>
>> Thanks,
>> Stratos
>>
>>  man/rbd.8  |    5 +++++
>>  src/rbd.cc |   54 +++++++++++++++++++++++++++++++++++++++++-------------
>>  2 files changed, 46 insertions(+), 13 deletions(-)
>>
>> diff --git a/man/rbd.8 b/man/rbd.8
>> index 6004244..7bf4c39 100644
>> --- a/man/rbd.8
>> +++ b/man/rbd.8
>> @@ -132,6 +132,11 @@ be open from more than one client at once, like during
>>  live migration of a virtual machine, or for use underneath
>>  a clustered filesystem.
>>  .UNINDENT
>> +.INDENT 0.0
>> +.TP
>> +.B \-\-json
>> +Dump output in json format (currently used only by showmapped).
>> +.UNINDENT
>>  .SH COMMANDS
>>  .INDENT 0.0
>>  .TP
>> diff --git a/src/rbd.cc b/src/rbd.cc
>> index 3db8978..82a72ba 100644
>> --- a/src/rbd.cc
>> +++ b/src/rbd.cc
>> @@ -47,6 +47,8 @@
>>  #include "include/rbd_types.h"
>>  #include "common/TextTable.h"
>>
>> +#include "common/Formatter.h"
>> +
>>  #if defined(__linux__)
>>  #include <linux/fs.h>
>>  #endif
>> @@ -126,7 +128,8 @@ void usage()
>>  "                               format 2 supports cloning\n"
>>  "  --id <username>              rados user (without 'client.' prefix) to authenticate as\n"
>>  "  --keyfile <path>             file containing secret key for use with cephx\n"
>> -"  --shared <tag>               take a shared (rather than exclusive) lock\n";
>> +"  --shared <tag>               take a shared (rather than exclusive) lock\n"
>> +"  --json                       dump output in json format (currently used only by showmapped)\n";
>>  }
>>
>>  static string feature_str(uint64_t features)
>> @@ -1198,7 +1201,7 @@ void do_closedir(DIR *dp)
>>      closedir(dp);
>>  }
>>
>> -static int do_kernel_showmapped()
>> +static int do_kernel_showmapped(bool dump_json)
>>  {
>>    int r;
>>    bool have_output = false;
>> @@ -1220,12 +1223,18 @@ static int do_kernel_showmapped()
>>      return r;
>>    }
>>
>> +  JSONFormatter jsf(false);
>>    TextTable tbl;
>> -  tbl.define_column("id", TextTable::LEFT, TextTable::LEFT);
>> -  tbl.define_column("pool", TextTable::LEFT, TextTable::LEFT);
>> -  tbl.define_column("image", TextTable::LEFT, TextTable::LEFT);
>> -  tbl.define_column("snap", TextTable::LEFT, TextTable::LEFT);
>> -  tbl.define_column("device", TextTable::LEFT, TextTable::LEFT);
>> +  if(dump_json == false) {
>> +    tbl.define_column("id", TextTable::LEFT, TextTable::LEFT);
>> +    tbl.define_column("pool", TextTable::LEFT, TextTable::LEFT);
>> +    tbl.define_column("image", TextTable::LEFT, TextTable::LEFT);
>> +    tbl.define_column("snap", TextTable::LEFT, TextTable::LEFT);
>> +    tbl.define_column("device", TextTable::LEFT, TextTable::LEFT);
>> +  }
>> +  else {
>> +    jsf.open_object_section("devices");
>> +  }
>>
>>    do {
>>      if (strcmp(dent->d_name, ".") == 0 || strcmp(dent->d_name, "..") == 0)
>> @@ -1262,13 +1271,30 @@ static int do_kernel_showmapped()
>>            << cpp_strerror(-r) << std::endl;
>>        continue;
>>      }
>> -
>> -    tbl << dent->d_name << pool << name << snap << dev << TextTable::endrow;
>> +
>> +    if (dump_json == false) {
>> +      tbl << dent->d_name << pool << name << snap << dev << TextTable::endrow;
>> +    }
>> +    else {
>> +      jsf.open_object_section(dent->d_name);
>> +      jsf.dump_string("pool", pool);
>> +      jsf.dump_string("name", name);
>> +      jsf.dump_string("snap", snap);
>> +      jsf.dump_string("device", dev);
>> +      jsf.close_section();
>> +    }
>>      have_output = true;
>>
>>    } while ((dent = readdir(device_dir.get())));
>> -  if (have_output)
>> -    cout << tbl;
>> +
>> +  if (dump_json == false) {
>> +    if (have_output)
>> +      cout << tbl;
>> +  }
>> +  else {
>> +    jsf.close_section();
>> +    jsf.flush(cout);
>> +  }
>>
>>    return 0;
>>  }
>> @@ -1505,7 +1531,7 @@ int main(int argc, const char **argv)
>>    const char *poolname = NULL;
>>    uint64_t size = 0;  // in bytes
>>    int order = 0;
>> -  bool format_specified = false;
>> +  bool format_specified = false, dump_json = false;
>>    int format = 1;
>>    uint64_t features = RBD_FEATURE_LAYERING;
>>    const char *imgname = NULL, *snapname = NULL, *destname = NULL,
>> @@ -1579,6 +1605,8 @@ int main(int argc, const char **argv)
>>        imgname = strdup(val.c_str());
>>      } else if (ceph_argparse_witharg(args, i, &val, "--shared", (char *)NULL)) {
>>        lock_tag = strdup(val.c_str());
>> +    } else if (ceph_argparse_flag(args, i, "--json", (char *) NULL)) {
>> +      dump_json = true;
>>      } else {
>>        ++i;
>>      }
>> @@ -2130,7 +2158,7 @@ if (!set_conf_param(v, p1, p2, p3)) { \
>>      break;
>>
>>    case OPT_SHOWMAPPED:
>> -    r = do_kernel_showmapped();
>> +    r = do_kernel_showmapped(dump_json);
>>      if (r < 0) {
>>        cerr << "rbd: showmapped failed: " << cpp_strerror(-r) << std::endl;
>>        return EXIT_FAILURE;
>> --
>> 1.7.10.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Stratos Psomadakis
<psomas@grnet.gr>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]

Re: [PATCH] rbd: Add --json flag for the showmapped command

[Josh Durgin]

On 12/13/2012 07:37 AM, Stratos Psomadakis wrote:
> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
> ---
> Hi Josh,
>
> This patch adds the '--json' flag to enable dumping the showmapped output in
> json format (as you suggested). I'm not sure if any other rbd subcommands could
> make use of this flag (so the patch is showmapped-specific atm).

Thanks, this looks good overall. Mainly some style nits below.

Other subcommands that could use json output would be:

info
ls [-l]
snap ls
lock list
children

> Comments?
>
> Thanks,
> Stratos
>
>   man/rbd.8  |    5 +++++
>   src/rbd.cc |   54 +++++++++++++++++++++++++++++++++++++++++-------------
>   2 files changed, 46 insertions(+), 13 deletions(-)
>
> diff --git a/man/rbd.8 b/man/rbd.8
> index 6004244..7bf4c39 100644
> --- a/man/rbd.8
> +++ b/man/rbd.8
> @@ -132,6 +132,11 @@ be open from more than one client at once, like during
>   live migration of a virtual machine, or for use underneath
>   a clustered filesystem.
>   .UNINDENT
> +.INDENT 0.0
> +.TP
> +.B \-\-json
> +Dump output in json format (currently used only by showmapped).
> +.UNINDENT
>   .SH COMMANDS
>   .INDENT 0.0
>   .TP

These man pages are actually generated from rst in ceph.git.
In this case the source file is docs/man/8/rbd.rst. Building
the documentation rebuilds the man pages as well [1], although
this could be more explicit in the docs.

If you send the rst change, I can rebuild the man pages.

> diff --git a/src/rbd.cc b/src/rbd.cc
> index 3db8978..82a72ba 100644
> --- a/src/rbd.cc
> +++ b/src/rbd.cc
> @@ -47,6 +47,8 @@
>   #include "include/rbd_types.h"
>   #include "common/TextTable.h"
>
> +#include "common/Formatter.h"
> +
>   #if defined(__linux__)
>   #include <linux/fs.h>
>   #endif
> @@ -126,7 +128,8 @@ void usage()
>   "                               format 2 supports cloning\n"
>   "  --id <username>              rados user (without 'client.' prefix) to authenticate as\n"
>   "  --keyfile <path>             file containing secret key for use with cephx\n"
> -"  --shared <tag>               take a shared (rather than exclusive) lock\n";
> +"  --shared <tag>               take a shared (rather than exclusive) lock\n"
> +"  --json                       dump output in json format (currently used only by showmapped)\n";
>   }
>
>   static string feature_str(uint64_t features)
> @@ -1198,7 +1201,7 @@ void do_closedir(DIR *dp)
>       closedir(dp);
>   }
>
> -static int do_kernel_showmapped()
> +static int do_kernel_showmapped(bool dump_json)
>   {
>     int r;
>     bool have_output = false;
> @@ -1220,12 +1223,18 @@ static int do_kernel_showmapped()
>       return r;
>     }
>
> +  JSONFormatter jsf(false);
>     TextTable tbl;
> -  tbl.define_column("id", TextTable::LEFT, TextTable::LEFT);
> -  tbl.define_column("pool", TextTable::LEFT, TextTable::LEFT);
> -  tbl.define_column("image", TextTable::LEFT, TextTable::LEFT);
> -  tbl.define_column("snap", TextTable::LEFT, TextTable::LEFT);
> -  tbl.define_column("device", TextTable::LEFT, TextTable::LEFT);
> +  if(dump_json == false) {

Missing space between if and (. Swapping the cases so if (dump_json)
came first would be a little nicer to read.

> +    tbl.define_column("id", TextTable::LEFT, TextTable::LEFT);
> +    tbl.define_column("pool", TextTable::LEFT, TextTable::LEFT);
> +    tbl.define_column("image", TextTable::LEFT, TextTable::LEFT);
> +    tbl.define_column("snap", TextTable::LEFT, TextTable::LEFT);
> +    tbl.define_column("device", TextTable::LEFT, TextTable::LEFT);
> +  }
> +  else {

} else {

> +    jsf.open_object_section("devices");
> +  }
>
>     do {
>       if (strcmp(dent->d_name, ".") == 0 || strcmp(dent->d_name, "..") == 0)
> @@ -1262,13 +1271,30 @@ static int do_kernel_showmapped()
>   	   << cpp_strerror(-r) << std::endl;
>         continue;
>       }
> -
> -    tbl << dent->d_name << pool << name << snap << dev << TextTable::endrow;
> +
> +    if (dump_json == false) {

same comment about swapping the cases

> +      tbl << dent->d_name << pool << name << snap << dev << TextTable::endrow;
> +    }
> +    else {

} else {

> +      jsf.open_object_section(dent->d_name);
> +      jsf.dump_string("pool", pool);
> +      jsf.dump_string("name", name);
> +      jsf.dump_string("snap", snap);
> +      jsf.dump_string("device", dev);
> +      jsf.close_section();
> +    }
>       have_output = true;
>
>     } while ((dent = readdir(device_dir.get())));
> -  if (have_output)
> -    cout << tbl;
> +
> +  if (dump_json == false) {

same comment about swapping the cases

> +    if (have_output)
> +      cout << tbl;
> +  }
> +  else {

} else {

> +    jsf.close_section();
> +    jsf.flush(cout);
> +  }
>
>     return 0;
>   }
> @@ -1505,7 +1531,7 @@ int main(int argc, const char **argv)
>     const char *poolname = NULL;
>     uint64_t size = 0;  // in bytes
>     int order = 0;
> -  bool format_specified = false;
> +  bool format_specified = false, dump_json = false;
>     int format = 1;
>     uint64_t features = RBD_FEATURE_LAYERING;
>     const char *imgname = NULL, *snapname = NULL, *destname = NULL,
> @@ -1579,6 +1605,8 @@ int main(int argc, const char **argv)
>         imgname = strdup(val.c_str());
>       } else if (ceph_argparse_witharg(args, i, &val, "--shared", (char *)NULL)) {
>         lock_tag = strdup(val.c_str());
> +    } else if (ceph_argparse_flag(args, i, "--json", (char *) NULL)) {
> +      dump_json = true;
>       } else {
>         ++i;
>       }
> @@ -2130,7 +2158,7 @@ if (!set_conf_param(v, p1, p2, p3)) { \
>       break;
>
>     case OPT_SHOWMAPPED:
> -    r = do_kernel_showmapped();
> +    r = do_kernel_showmapped(dump_json);
>       if (r < 0) {
>         cerr << "rbd: showmapped failed: " << cpp_strerror(-r) << std::endl;
>         return EXIT_FAILURE;
>

It'd be nice to report an error and fail if the --json option (or
whatever it ends up being) is provided for a command that doesn't use
it.

Josh

[1] http://ceph.com/docs/master/dev/generatedocs/

Re: [PATCH] rbd: Add --json flag for the showmapped command

[Josh Durgin]

On 12/14/2012 12:57 AM, Stratos Psomadakis wrote:
> On 12/13/2012 07:17 PM, Yehuda Sadeh wrote:
>> On Thu, Dec 13, 2012 at 7:37 AM, Stratos Psomadakis <psomas@grnet.gr> wrote:
>>> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
>>> ---
>>> Hi Josh,
>>>
>>> This patch adds the '--json' flag to enable dumping the showmapped output in
>> I think that it should be "--format=json" rather than --json. This
>> will make it more in line with other tools. Also, then you could have
>> the unpopular --format=xml.
>
> Hi,
>
> the problem is that --format is arleady used in rbd (to select the rbd
> image format). So, you could either use --output-format= for
> plain/json/xml output (but then you would be inconsistent wrt the other
> tools), or change --format to --image-format (or something like that),
> and use --format to select the output formatting.

We could deprecate --format in favor of --image-format, but maintain
backwards compatibility	by accepting the string 1 or 2 for --format
still. Maybe not pretty, but it lets us be consistent without changing
existing usage.

Josh

Re: [PATCH] rbd: Add --json flag for the showmapped command

[Yehuda Sadeh]

On Mon, Dec 17, 2012 at 9:32 AM, Josh Durgin <josh.durgin@inktank.com> wrote:
> On 12/14/2012 12:57 AM, Stratos Psomadakis wrote:
>>
>> On 12/13/2012 07:17 PM, Yehuda Sadeh wrote:
>>>
>>> On Thu, Dec 13, 2012 at 7:37 AM, Stratos Psomadakis <psomas@grnet.gr>
>>> wrote:
>>>>
>>>> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
>>>> ---
>>>> Hi Josh,
>>>>
>>>> This patch adds the '--json' flag to enable dumping the showmapped
>>>> output in
>>>
>>> I think that it should be "--format=json" rather than --json. This
>>> will make it more in line with other tools. Also, then you could have
>>> the unpopular --format=xml.
>>
>>
>> Hi,
>>
>> the problem is that --format is arleady used in rbd (to select the rbd
>> image format). So, you could either use --output-format= for
>> plain/json/xml output (but then you would be inconsistent wrt the other
>> tools), or change --format to --image-format (or something like that),
>> and use --format to select the output formatting.
>
>
> We could deprecate --format in favor of --image-format, but maintain
> backwards compatibility by accepting the string 1 or 2 for --format
> still. Maybe not pretty, but it lets us be consistent without changing
> existing usage.
>
That would work, but we should dump a noticeable warning whenever it's
being used.

Re: [PATCH] rbd: Add --json flag for the showmapped command

[Josh Durgin]

On 12/17/2012 09:35 AM, Yehuda Sadeh wrote:
> On Mon, Dec 17, 2012 at 9:32 AM, Josh Durgin <josh.durgin@inktank.com> wrote:
>> On 12/14/2012 12:57 AM, Stratos Psomadakis wrote:
>>>
>>> On 12/13/2012 07:17 PM, Yehuda Sadeh wrote:
>>>>
>>>> On Thu, Dec 13, 2012 at 7:37 AM, Stratos Psomadakis <psomas@grnet.gr>
>>>> wrote:
>>>>>
>>>>> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
>>>>> ---
>>>>> Hi Josh,
>>>>>
>>>>> This patch adds the '--json' flag to enable dumping the showmapped
>>>>> output in
>>>>
>>>> I think that it should be "--format=json" rather than --json. This
>>>> will make it more in line with other tools. Also, then you could have
>>>> the unpopular --format=xml.
>>>
>>>
>>> Hi,
>>>
>>> the problem is that --format is arleady used in rbd (to select the rbd
>>> image format). So, you could either use --output-format= for
>>> plain/json/xml output (but then you would be inconsistent wrt the other
>>> tools), or change --format to --image-format (or something like that),
>>> and use --format to select the output formatting.
>>
>>
>> We could deprecate --format in favor of --image-format, but maintain
>> backwards compatibility by accepting the string 1 or 2 for --format
>> still. Maybe not pretty, but it lets us be consistent without changing
>> existing usage.
>>
> That would work, but we should dump a noticeable warning whenever it's
> being used.

Sounds good to me.

[PATCH v2] rbd: Support plain/json/xml output formatting

[Stratos Psomadakis]

This patch renames the --format option to --image-format, for specyfing the RBD
image format, and uses --format to specify the output formating (to be
consistent with the other ceph tools). To avoid breaking backwards compatibility
with existing scripts, rbd will still accept --format [1|2] for the image
format, but will print a warning message, noting its use is deprecated.

The rbd subcommands that support the new --format option are : ls, info, snap
list, children, showmapped, lock list.

Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
---
Hi,

this is the updated version of the patch. I renamed --format option to
 --image-format, and used --format to specify the output formatting, as you
suggested.

I also implemented some basic error checking on the --format input, and modified
the rbd subcommands you mentioned, to support plain/json/xml output formatting.
Although, I'm not sure if the json and xml output of those commands is what
you'd want.

The style issues should also be resolved now.

Let me know what you think.

Thanks,
Stratos

 doc/man/8/rbd.rst |    6 +-
 src/rbd.cc        |  424 ++++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 325 insertions(+), 105 deletions(-)

diff --git a/doc/man/8/rbd.rst b/doc/man/8/rbd.rst
index e6a02b0..3446364 100644
--- a/doc/man/8/rbd.rst
+++ b/doc/man/8/rbd.rst
@@ -41,7 +41,7 @@ Options
 Parameters
 ==========
 
-.. option:: --format format
+.. option:: --image-format format
 
    Specifies which object layout to use. The default is 1.
 
@@ -100,6 +100,10 @@ Parameters
    live migration of a virtual machine, or for use underneath
    a clustered filesystem.
 
+.. option:: --format format
+
+   Specifies output formatting (default: plain, json, xml)
+
 
 Commands
 ========
diff --git a/src/rbd.cc b/src/rbd.cc
index 3920d4b..f0487bc 100644
--- a/src/rbd.cc
+++ b/src/rbd.cc
@@ -19,6 +19,7 @@
 #include "auth/KeyRing.h"
 #include "common/errno.h"
 #include "common/ceph_argparse.h"
+#include "common/strtol.h"
 #include "global/global_init.h"
 #include "common/safe_io.h"
 #include "common/secret.h"
@@ -47,6 +48,8 @@
 #include "include/rbd_types.h"
 #include "common/TextTable.h"
 
+#include "common/Formatter.h"
+
 #if defined(__linux__)
 #include <linux/fs.h>
 #endif
@@ -112,21 +115,22 @@ void usage()
 "individual pieces of names with -p/--pool, --image, and/or --snap.\n"
 "\n"
 "Other input options:\n"
-"  -p, --pool <pool>            source pool name\n"
-"  --image <image-name>         image name\n"
-"  --dest <image-name>          destination [pool and] image name\n"
-"  --snap <snap-name>           snapshot name\n"
-"  --dest-pool <name>           destination pool name\n"
-"  --path <path-name>           path name for import/export\n"
-"  --size <size in MB>          size of image for create and resize\n"
-"  --order <bits>               the object size in bits; object size will be\n"
-"                               (1 << order) bytes. Default is 22 (4 MB).\n"
-"  --format <format-number>     format to use when creating an image\n"
-"                               format 1 is the original format (default)\n"
-"                               format 2 supports cloning\n"
-"  --id <username>              rados user (without 'client.' prefix) to authenticate as\n"
-"  --keyfile <path>             file containing secret key for use with cephx\n"
-"  --shared <tag>               take a shared (rather than exclusive) lock\n";
+"  -p, --pool <pool>                  source pool name\n"
+"  --image <image-name>               image name\n"
+"  --dest <image-name>                destination [pool and] image name\n"
+"  --snap <snap-name>                 snapshot name\n"
+"  --dest-pool <name>                 destination pool name\n"
+"  --path <path-name>                 path name for import/export\n"
+"  --size <size in MB>                size of image for create and resize\n"
+"  --order <bits>                     the object size in bits; object size will be\n"
+"                                     (1 << order) bytes. Default is 22 (4 MB).\n"
+"  --image-format <format-number>     format to use when creating an image\n"
+"                                     format 1 is the original format (default)\n"
+"                                     format 2 supports cloning\n"
+"  --id <username>                    rados user (without 'client.' prefix) to authenticate as\n"
+"  --keyfile <path>                   file containing secret key for use with cephx\n"
+"  --shared <tag>                     take a shared (rather than exclusive) lock\n"
+"  --format <output-format>           output format (default: plain, json, xml)\n";
 }
 
 static string feature_str(uint64_t features)
@@ -169,28 +173,62 @@ struct MyProgressContext : public librbd::ProgressContext {
   }
 };
 
-static int do_list(librbd::RBD &rbd, librados::IoCtx& io_ctx, bool lflag)
+static int get_outfmt(const char *output_format, Formatter **f)
+{
+  if (!strcmp(output_format, "json"))
+    *f = new JSONFormatter(false);
+  else if (!strcmp(output_format, "xml"))
+    *f = new XMLFormatter(false);
+  else if (strcmp(output_format, "plain")) {
+    cerr << "rbd unknown format '" << output_format << "'" << std::endl;
+    return -EINVAL;
+  }
+
+  return 0;
+}
+
+static int do_list(librbd::RBD &rbd, librados::IoCtx& io_ctx, bool lflag, const char *output_format)
 {
   std::vector<string> names;
   int r = rbd.list(io_ctx, names);
   if (r < 0 || (names.size() == 0))
     return r;
 
+  Formatter *f = 0;
+
+  r = get_outfmt(output_format, &f);
+  if (r < 0)
+    return r;
+
   if (!lflag) {
+    if (f)
+      f->open_array_section("images");
     for (std::vector<string>::const_iterator i = names.begin();
        i != names.end(); ++i) {
-      cout << *i << std::endl;
+       if (f)
+         f->dump_string("name", string(*i).c_str());
+       else
+         cout << *i << std::endl;
+    }
+    if (f) {
+      f->close_section();
+      f->flush(cout);
     }
     return 0;
   }
 
   TextTable tbl;
-  tbl.define_column("NAME", TextTable::LEFT, TextTable::LEFT);
-  tbl.define_column("SIZE", TextTable::RIGHT, TextTable::RIGHT);
-  tbl.define_column("PARENT", TextTable::LEFT, TextTable::LEFT);
-  tbl.define_column("FMT", TextTable::RIGHT, TextTable::RIGHT);
-  tbl.define_column("PROT", TextTable::LEFT, TextTable::LEFT);
-  tbl.define_column("LOCK", TextTable::LEFT, TextTable::LEFT);
+
+  if (f)
+    f->open_object_section("images");
+  else {
+    tbl.define_column("NAME", TextTable::LEFT, TextTable::LEFT);
+    tbl.define_column("SIZE", TextTable::RIGHT, TextTable::RIGHT);
+    tbl.define_column("PARENT", TextTable::LEFT, TextTable::LEFT);
+    tbl.define_column("FMT", TextTable::RIGHT, TextTable::RIGHT);
+    tbl.define_column("PROT", TextTable::LEFT, TextTable::LEFT);
+    tbl.define_column("LOCK", TextTable::LEFT, TextTable::LEFT);
+  }
 
   string pool, image, snap, parent;
 
@@ -238,13 +276,23 @@ static int do_list(librbd::RBD &rbd, librados::IoCtx& io_ctx, bool lflag)
       lockstr = (exclusive) ? "excl" : "shr";
     }
 
-    tbl << *i
-	<< stringify(si_t(info.size))
-	<< parent
-	<< ((old_format) ? '1' : '2')
-	<< ""				// protect doesn't apply to images
-	<< lockstr
-	<< TextTable::endrow;
+    if (f) {
+      f->open_object_section(string(*i).c_str());
+      f->dump_string("size", stringify(si_t(info.size)));
+      f->dump_string("parent", parent);
+      f->dump_string("fmt", (old_format) ? "1" : "2");
+      f->dump_string("prot", "");  // protect doesn't apply to images
+      f->dump_string("lock", lockstr);
+      f->close_section();
+    } else {
+      tbl << *i
+	  << stringify(si_t(info.size))
+	  << parent
+	  << ((old_format) ? '1' : '2')
+	  << ""				// protect doesn't apply to images
+	  << lockstr
+	  << TextTable::endrow;
+    }
 
     vector<librbd::snap_info_t> snaplist;
     if (im.snap_list(snaplist) >= 0 && !snaplist.empty()) {
@@ -259,17 +307,32 @@ static int do_list(librbd::RBD &rbd, librados::IoCtx& io_ctx, bool lflag)
 	if (im.parent_info(&pool, &image, &snap) >= 0) {
 	  parent = pool + "/" + image + "@" + snap;
 	}
-        tbl << *i + "@" + s->name
-	    << stringify(si_t(s->size))
-	    << parent
-	    << ((old_format) ? '1' : '2')
-	    << (is_protected ? "yes" : "")
-	    << "" 			// locks don't apply to snaps
-	    << TextTable::endrow;
+        if (f) {
+          f->open_object_section((*i + "@" + s->name).c_str());
+          f->dump_string("size", stringify(si_t(s->size)));
+          f->dump_string("parent", parent);
+          f->dump_string("fmt", (old_format) ? "1" : "2");
+          f->dump_string("prot", is_protected ? "yes" : "");
+          f->dump_string("lock", "");  // locks don't apply to snaps
+          f->close_section();
+        } else {
+          tbl << *i + "@" + s->name
+	      << stringify(si_t(s->size))
+	      << parent
+	      << ((old_format) ? '1' : '2')
+	      << (is_protected ? "yes" : "")
+	      << "" 			// locks don't apply to snaps
+	      << TextTable::endrow;
+        }
       }
     }
   }
-  cout << tbl;
+  if (f) {
+    f->close_section();
+    f->flush(cout);
+  } else
+    cout << tbl;
+
   return 0;
 }
 
@@ -337,15 +400,21 @@ static int do_rename(librbd::RBD &rbd, librados::IoCtx& io_ctx,
 }
 
 static int do_show_info(const char *imgname, librbd::Image& image,
-			const char *snapname)
+			const char *snapname, const char *output_format)
 {
   librbd::image_info_t info;
   string parent_pool, parent_name, parent_snapname;
   uint8_t old_format;
   uint64_t overlap, features;
   bool snap_protected;
+  Formatter *f = 0;
+  int r;
+
+  r = get_outfmt(output_format, &f);
+  if (r < 0)
+    return r;
 
-  int r = image.stat(info, sizeof(info));
+  r = image.stat(info, sizeof(info));
   if (r < 0)
     return r;
 
@@ -367,41 +436,73 @@ static int do_show_info(const char *imgname, librbd::Image& image,
       return r;
   }
 
-  cout << "rbd image '" << imgname << "':\n"
-       << "\tsize " << prettybyte_t(info.size) << " in "
-       << info.num_objs << " objects"
-       << std::endl
-       << "\torder " << info.order
-       << " (" << prettybyte_t(info.obj_size) << " objects)"
-       << std::endl
-       << "\tblock_name_prefix: " << info.block_name_prefix
-       << std::endl
-       << "\tformat: " << (old_format ? "1" : "2")
-       << std::endl;
+  if (f) {
+    f->open_object_section("image");
+    f->dump_string("name", imgname);
+    f->dump_string("size", stringify(prettybyte_t(info.size)));
+    f->dump_int("objects", info.num_objs);
+    f->dump_int("order", info.order);
+    f->dump_string("obj_size", stringify(prettybyte_t(info.obj_size)));
+    f->dump_string("block_name_prefix", info.block_name_prefix);
+    f->dump_string("format", (old_format ? "1" : "2"));
+  } else {
+    cout << "rbd image '" << imgname << "':\n"
+         << "\tsize " << prettybyte_t(info.size) << " in "
+         << info.num_objs << " objects"
+         << std::endl
+         << "\torder " << info.order
+         << " (" << prettybyte_t(info.obj_size) << " objects)"
+         << std::endl
+         << "\tblock_name_prefix: " << info.block_name_prefix
+         << std::endl
+         << "\tformat: " << (old_format ? "1" : "2")
+         << std::endl;
+  }
+
   if (!old_format) {
-    cout << "\tfeatures: " << feature_str(features) << std::endl;
+    if (f)
+      f->dump_string("features", feature_str(features));
+    else
+      cout << "\tfeatures: " << feature_str(features) << std::endl;
   }
 
   // snapshot info, if present
   if (snapname) {
-    cout << "\tprotected: " << (snap_protected ? "True" : "False")
-	 << std::endl;
+    if (f)
+      f->dump_string("protected", stringify(snap_protected));
+    else
+      cout << "\tprotected: " << (snap_protected ? "True" : "False")
+	   << std::endl;
   }
 
   // parent info, if present
   if ((image.parent_info(&parent_pool, &parent_name, &parent_snapname) == 0) &&
       parent_name.length() > 0) {
-
-    cout << "\tparent: " << parent_pool << "/" << parent_name
-	 << "@" << parent_snapname << std::endl;
-    cout << "\toverlap: " << prettybyte_t(overlap) << std::endl;
+    if (f) {
+      f->dump_string("parnet", parent_pool + "/" + parent_name);
+      f->dump_string("overlap", stringify(prettybyte_t(overlap)));
+    } else {
+      cout << "\tparent: " << parent_pool << "/" << parent_name
+	   << "@" << parent_snapname << std::endl;
+      cout << "\toverlap: " << prettybyte_t(overlap) << std::endl;
+    }
   }
 
   // striping info, if feature is set
   if (features & RBD_FEATURE_STRIPINGV2) {
-    cout << "\tstripe unit: " << prettybyte_t(image.get_stripe_unit()) << std::endl
-	 << "\tstripe count: " << image.get_stripe_count() << std::endl;
+    if (f) {
+      f->dump_string("stripe_unit", stringify(prettybyte_t(image.get_stripe_unit())));
+      f->dump_string("stripe_count", stringify(image.get_stripe_count()));
+    } else
+      cout << "\tstripe unit: " << prettybyte_t(image.get_stripe_unit()) << std::endl
+	   << "\tstripe count: " << image.get_stripe_count() << std::endl;
   }
+
+  if (f) {
+    f->close_section();
+    f->flush(cout);
+  }
+
   return 0;
 }
 
@@ -429,24 +530,49 @@ static int do_resize(librbd::Image& image, uint64_t size)
   return 0;
 }
 
-static int do_list_snaps(librbd::Image& image)
+static int do_list_snaps(librbd::Image& image, const char *output_format)
 {
   std::vector<librbd::snap_info_t> snaps;
-  int r = image.snap_list(snaps);
+  Formatter *f = 0;
+  TextTable t;
+  int r;
+
+  r = get_outfmt(output_format, &f);
+  if (r < 0)
+    return r;
+
+  r  = image.snap_list(snaps);
   if (r < 0 || snaps.empty())
     return r;
 
-  TextTable t;
-  t.define_column("SNAPID", TextTable::RIGHT, TextTable::RIGHT);
-  t.define_column("NAME", TextTable::LEFT, TextTable::LEFT);
-  t.define_column("SIZE", TextTable::RIGHT, TextTable::RIGHT);
+  if (f)
+    f->open_object_section("snaps");
+  else {
+    t.define_column("SNAPID", TextTable::RIGHT, TextTable::RIGHT);
+    t.define_column("NAME", TextTable::LEFT, TextTable::LEFT);
+    t.define_column("SIZE", TextTable::RIGHT, TextTable::RIGHT);
+  }
 
   for (std::vector<librbd::snap_info_t>::iterator s = snaps.begin();
        s != snaps.end(); ++s) {
-    t << s->id << s->name << stringify(prettybyte_t(s->size))
-      << TextTable::endrow;
+    if (f) {
+      f->open_object_section(stringify(s->id).c_str());
+      f->dump_string("name", s->name);
+      f->dump_string("size", stringify(prettybyte_t(s->size)));
+      f->close_section();
+    } else {
+      t << s->id << s->name << stringify(prettybyte_t(s->size))
+        << TextTable::endrow;
+    }
+  }
+
+  if (f) {
+    f->close_section();
+    f->flush(cout);
   }
-  cout << t;
+  else
+    cout << t;
+
   return 0;
 }
 
@@ -521,47 +647,91 @@ static int do_unprotect_snap(librbd::Image& image, const char *snapname)
   return 0;
 }
 
-static int do_list_children(librbd::Image &image)
+static int do_list_children(librbd::Image &image, const char *output_format)
 {
   set<pair<string, string> > children;
-  int r = image.list_children(&children);
+  Formatter *f = 0;
+  int r;
+
+  r = get_outfmt(output_format, &f);
+  if (r < 0)
+    return r;
+
+  r = image.list_children(&children);
   if (r < 0)
     return r;
 
+  if (f)
+    f->open_array_section("children");
+
   for (set<pair<string, string> >::const_iterator child_it = children.begin();
        child_it != children.end(); child_it++) {
-    cout << child_it->first << "/" << child_it->second << std::endl;
+    if (f)
+      f->dump_string("child", child_it->first + "/" + child_it->second);
+    else
+      cout << child_it->first << "/" << child_it->second << std::endl;
   }
+
+  if (f) {
+    f->close_section();
+    f->flush(cout);
+  }
+
   return 0;
 }
 
-static int do_lock_list(librbd::Image& image)
+static int do_lock_list(librbd::Image& image, const char *output_format)
 {
   list<librbd::locker_t> lockers;
   bool exclusive;
   string tag;
-  int r = image.list_lockers(&lockers, &exclusive, &tag);
+  Formatter *f = 0;
+  TextTable tbl;
+  int r;
+
+  r = get_outfmt(output_format, &f);
+  if (r < 0)
+    return r;
+
+  r = image.list_lockers(&lockers, &exclusive, &tag);
   if (r < 0)
     return r;
 
   if (lockers.size()) {
     bool one = (lockers.size() == 1);
-    cout << "There " << (one ? "is " : "are ") << lockers.size()
-	 << (exclusive ? " exclusive" : " shared")
-	 << " lock" << (one ? "" : "s") << " on this image.\n";
-    if (!exclusive)
-      cout << "Lock tag: " << tag << "\n";
 
-    TextTable tbl;
-    tbl.define_column("Locker", TextTable::LEFT, TextTable::LEFT);
-    tbl.define_column("ID", TextTable::LEFT, TextTable::LEFT);
-    tbl.define_column("Address", TextTable::LEFT, TextTable::LEFT);
+    if (!f) {
+      cout << "There " << (one ? "is " : "are ") << lockers.size()
+	   << (exclusive ? " exclusive" : " shared")
+	   << " lock" << (one ? "" : "s") << " on this image.\n";
+      if (!exclusive)
+        cout << "Lock tag: " << tag << "\n";
+    }
+
+    if (f)
+      f->open_object_section("locks");
+    else {
+      tbl.define_column("Locker", TextTable::LEFT, TextTable::LEFT);
+      tbl.define_column("ID", TextTable::LEFT, TextTable::LEFT);
+      tbl.define_column("Address", TextTable::LEFT, TextTable::LEFT);
+    }
 
     for (list<librbd::locker_t>::const_iterator it = lockers.begin();
 	 it != lockers.end(); ++it) {
+      if (f) {
+        f->open_object_section(it->cookie.c_str());
+        f->dump_string("locker", it->client);
+        f->dump_string("address", it->address);
+        f->close_section();
+      } else
       tbl << it->client << it->cookie << it->address << TextTable::endrow;
     }
-    cout << tbl;
+
+    if (f) {
+      f->close_section();
+      f->flush(cout);
+    } else
+      cout << tbl;
   }
   return 0;
 }
@@ -1210,10 +1380,17 @@ void do_closedir(DIR *dp)
     closedir(dp);
 }
 
-static int do_kernel_showmapped()
+static int do_kernel_showmapped(const char *output_format)
 {
   int r;
   bool have_output = false;
+  Formatter *f = 0;
+  TextTable tbl;
+
+  r = get_outfmt(output_format, &f);
+  if (r < 0)
+    return r;
+
   const char *devices_path = "/sys/bus/rbd/devices";
   std::tr1::shared_ptr<DIR> device_dir(opendir(devices_path), do_closedir);
   if (!device_dir.get()) {
@@ -1232,12 +1409,15 @@ static int do_kernel_showmapped()
     return r;
   }
 
-  TextTable tbl;
-  tbl.define_column("id", TextTable::LEFT, TextTable::LEFT);
-  tbl.define_column("pool", TextTable::LEFT, TextTable::LEFT);
-  tbl.define_column("image", TextTable::LEFT, TextTable::LEFT);
-  tbl.define_column("snap", TextTable::LEFT, TextTable::LEFT);
-  tbl.define_column("device", TextTable::LEFT, TextTable::LEFT);
+  if (f)
+    f->open_object_section("devices");
+  else {
+    tbl.define_column("id", TextTable::LEFT, TextTable::LEFT);
+    tbl.define_column("pool", TextTable::LEFT, TextTable::LEFT);
+    tbl.define_column("image", TextTable::LEFT, TextTable::LEFT);
+    tbl.define_column("snap", TextTable::LEFT, TextTable::LEFT);
+    tbl.define_column("device", TextTable::LEFT, TextTable::LEFT);
+  }
 
   do {
     if (strcmp(dent->d_name, ".") == 0 || strcmp(dent->d_name, "..") == 0)
@@ -1275,12 +1455,27 @@ static int do_kernel_showmapped()
       continue;
     }
 
-    tbl << dent->d_name << pool << name << snap << dev << TextTable::endrow;
+    if (f) {
+      f->open_object_section(dent->d_name);
+      f->dump_string("pool", pool);
+      f->dump_string("name", name);
+      f->dump_string("snap", snap);
+      f->dump_string("device", dev);
+      f->close_section();
+    } else {
+      tbl << dent->d_name << pool << name << snap << dev << TextTable::endrow;
+    }
     have_output = true;
 
   } while ((dent = readdir(device_dir.get())));
-  if (have_output)
-    cout << tbl;
+
+  if (f) {
+    f->close_section();
+    f->flush(cout);
+  } else {
+    if (have_output)
+      cout << tbl;
+  }
 
   return 0;
 }
@@ -1517,13 +1712,13 @@ int main(int argc, const char **argv)
   const char *poolname = NULL;
   uint64_t size = 0;  // in bytes
   int order = 0;
-  bool format_specified = false;
+  bool format_specified = false, output_format_specified = false;
   int format = 1;
   uint64_t features = RBD_FEATURE_LAYERING;
   const char *imgname = NULL, *snapname = NULL, *destname = NULL,
     *dest_poolname = NULL, *dest_snapname = NULL, *path = NULL,
     *devpath = NULL, *lock_cookie = NULL, *lock_client = NULL,
-    *lock_tag = NULL;
+    *lock_tag = NULL, *output_format = "plain";
   bool lflag = false;
   long long stripe_unit = 0, stripe_count = 0;
   long long bench_io_size = 4096, bench_io_threads = 16, bench_bytes = 1 << 30;
@@ -1544,7 +1739,7 @@ int main(int argc, const char **argv)
     } else if (ceph_argparse_flag(args, i, "--new-format", (char*)NULL)) {
       format = 2;
       format_specified = true;
-    } else if (ceph_argparse_withint(args, i, &format, &err, "--format",
+    } else if (ceph_argparse_withint(args, i, &format, &err, "--image-format",
 				     (char*)NULL)) {
       if (!err.str().empty()) {
 	cerr << "rbd: " << err.str() << std::endl;
@@ -1591,6 +1786,19 @@ int main(int argc, const char **argv)
       imgname = strdup(val.c_str());
     } else if (ceph_argparse_witharg(args, i, &val, "--shared", (char *)NULL)) {
       lock_tag = strdup(val.c_str());
+    } else if (ceph_argparse_witharg(args, i, &val, "--format", (char *) NULL)) {
+      std::string err;
+      long long ret = strict_strtoll(val.c_str(), 10, &err);
+      if (err.empty()) {
+        format = ret;
+        format_specified = true;
+        cerr << "rbd: using --format for specifying the rbd image format is deprecated,"
+             << " use --image-format instead"
+             << std::endl;
+      } else {
+        output_format = strdup(val.c_str());
+        output_format_specified = true;
+      }
     } else {
       ++i;
     }
@@ -1692,11 +1900,19 @@ if (!set_conf_param(v, p1, p2, p3)) { \
   }
 
   if (format_specified && opt_cmd != OPT_IMPORT && opt_cmd != OPT_CREATE) {
-    cerr << "rbd: format can only be set when "
+    cerr << "rbd: image format can only be set when "
 	 << "creating or importing an image" << std::endl;
     return EXIT_FAILURE;
   }
 
+  if (output_format_specified && opt_cmd != OPT_SHOWMAPPED && opt_cmd != OPT_INFO &&
+      opt_cmd != OPT_LIST && opt_cmd != OPT_SNAP_LIST && opt_cmd != OPT_LOCK_LIST &&
+      opt_cmd != OPT_CHILDREN) {
+    cerr << "rbd: command doesn't use output formatting"
+         << std::endl;
+    return EXIT_FAILURE;
+  }
+
   if (format_specified) {
     if (format < 1 || format > 2) {
       cerr << "rbd: format must be 1 or 2" << std::endl;
@@ -1883,7 +2099,7 @@ if (!set_conf_param(v, p1, p2, p3)) { \
 
   switch (opt_cmd) {
   case OPT_LIST:
-    r = do_list(rbd, io_ctx, lflag);
+    r = do_list(rbd, io_ctx, lflag, output_format);
     if (r < 0) {
       switch (r) {
       case -ENOENT:
@@ -1947,7 +2163,7 @@ if (!set_conf_param(v, p1, p2, p3)) { \
     break;
 
   case OPT_INFO:
-    r = do_show_info(imgname, image, snapname);
+    r = do_show_info(imgname, image, snapname, output_format);
     if (r < 0) {
       cerr << "rbd: info: " << cpp_strerror(-r) << std::endl;
       return EXIT_FAILURE;
@@ -1988,7 +2204,7 @@ if (!set_conf_param(v, p1, p2, p3)) { \
       cerr << "rbd: snap list requires an image parameter" << std::endl;
       return EXIT_FAILURE;
     }
-    r = do_list_snaps(image);
+    r = do_list_snaps(image, output_format);
     if (r < 0) {
       cerr << "rbd: failed to list snapshots: " << cpp_strerror(-r)
 	   << std::endl;
@@ -2077,7 +2293,7 @@ if (!set_conf_param(v, p1, p2, p3)) { \
     break;
 
   case OPT_CHILDREN:
-    r = do_list_children(image);
+    r = do_list_children(image, output_format);
     if (r < 0) {
       cerr << "rbd: listing children failed: " << cpp_strerror(-r) << std::endl;
       return EXIT_FAILURE;
@@ -2142,7 +2358,7 @@ if (!set_conf_param(v, p1, p2, p3)) { \
     break;
 
   case OPT_SHOWMAPPED:
-    r = do_kernel_showmapped();
+    r = do_kernel_showmapped(output_format);
     if (r < 0) {
       cerr << "rbd: showmapped failed: " << cpp_strerror(-r) << std::endl;
       return EXIT_FAILURE;
@@ -2150,7 +2366,7 @@ if (!set_conf_param(v, p1, p2, p3)) { \
     break;
 
   case OPT_LOCK_LIST:
-    r = do_lock_list(image);
+    r = do_lock_list(image, output_format);
     if (r < 0) {
       cerr << "rbd: listing locks failed: " << cpp_strerror(r) << std::endl;
       return EXIT_FAILURE;
-- 
1.7.10.4

Re: [PATCH v2] rbd: Support plain/json/xml output formatting

[Josh Durgin]

On 12/19/2012 04:17 AM, Stratos Psomadakis wrote:
> This patch renames the --format option to --image-format, for specyfing the RBD
> image format, and uses --format to specify the output formating (to be
> consistent with the other ceph tools). To avoid breaking backwards compatibility
> with existing scripts, rbd will still accept --format [1|2] for the image
> format, but will print a warning message, noting its use is deprecated.
>
> The rbd subcommands that support the new --format option are : ls, info, snap
> list, children, showmapped, lock list.
>
> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
> ---
> Hi,
>
> this is the updated version of the patch. I renamed --format option to
>   --image-format, and used --format to specify the output formatting, as you
> suggested.
>
> I also implemented some basic error checking on the --format input, and modified
> the rbd subcommands you mentioned, to support plain/json/xml output formatting.
> Although, I'm not sure if the json and xml output of those commands is what
> you'd want.
>
> The style issues should also be resolved now.
>
> Let me know what you think.

There were still some style issues (missing braces, long lines, and
spaces instead of tabs), but I squashed fixes to those into the
wip-rbd-formatted-output branch.

I changed the output a bit to uses ints, arrays, etc. where it
seemed appropriate. The tests aren't quite done yet, but does
this new json output look good to you?

Josh

Re: [PATCH v2] rbd: Support plain/json/xml output formatting

[Stratos Psomadakis]

[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]

On 01/05/2013 01:50 AM, Josh Durgin wrote:
> On 12/19/2012 04:17 AM, Stratos Psomadakis wrote:
>> This patch renames the --format option to --image-format, for
>> specyfing the RBD
>> image format, and uses --format to specify the output formating (to be
>> consistent with the other ceph tools). To avoid breaking backwards
>> compatibility
>> with existing scripts, rbd will still accept --format [1|2] for the
>> image
>> format, but will print a warning message, noting its use is deprecated.
>>
>> The rbd subcommands that support the new --format option are : ls,
>> info, snap
>> list, children, showmapped, lock list.
>>
>> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
>> ---
>> Hi,
>>
>> this is the updated version of the patch. I renamed --format option to
>>   --image-format, and used --format to specify the output formatting,
>> as you
>> suggested.
>>
>> I also implemented some basic error checking on the --format input,
>> and modified
>> the rbd subcommands you mentioned, to support plain/json/xml output
>> formatting.
>> Although, I'm not sure if the json and xml output of those commands
>> is what
>> you'd want.
>>
>> The style issues should also be resolved now.
>>
>> Let me know what you think.
>
> There were still some style issues (missing braces, long lines, and
> spaces instead of tabs), but I squashed fixes to those into the
> wip-rbd-formatted-output branch.
>
> I changed the output a bit to uses ints, arrays, etc. where it
> seemed appropriate. The tests aren't quite done yet, but does
> this new json output look good to you?
>
> Josh

Hi,

Sorry for the delay. Thanks for resolving the style issues and cleaning
this up.

Wrt the json output, I think it's fine as it is now.

Just a minor comment. The --pretty-format option doesn't seem to be
documented (in either rbd --help or rbd man page). Other than that, I
don't see any other issues.

Thanks,
Stratos

-- 
Stratos Psomadakis
<psomas@grnet.gr>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH v2] rbd: Support plain/json/xml output formatting

[Josh Durgin]

On 01/14/2013 05:19 AM, Stratos Psomadakis wrote:
> On 01/05/2013 01:50 AM, Josh Durgin wrote:
>> On 12/19/2012 04:17 AM, Stratos Psomadakis wrote:
>>> This patch renames the --format option to --image-format, for
>>> specyfing the RBD
>>> image format, and uses --format to specify the output formating (to be
>>> consistent with the other ceph tools). To avoid breaking backwards
>>> compatibility
>>> with existing scripts, rbd will still accept --format [1|2] for the
>>> image
>>> format, but will print a warning message, noting its use is deprecated.
>>>
>>> The rbd subcommands that support the new --format option are : ls,
>>> info, snap
>>> list, children, showmapped, lock list.
>>>
>>> Signed-off-by: Stratos Psomadakis <psomas@grnet.gr>
>>> ---
>>> Hi,
>>>
>>> this is the updated version of the patch. I renamed --format option to
>>>    --image-format, and used --format to specify the output formatting,
>>> as you
>>> suggested.
>>>
>>> I also implemented some basic error checking on the --format input,
>>> and modified
>>> the rbd subcommands you mentioned, to support plain/json/xml output
>>> formatting.
>>> Although, I'm not sure if the json and xml output of those commands
>>> is what
>>> you'd want.
>>>
>>> The style issues should also be resolved now.
>>>
>>> Let me know what you think.
>>
>> There were still some style issues (missing braces, long lines, and
>> spaces instead of tabs), but I squashed fixes to those into the
>> wip-rbd-formatted-output branch.
>>
>> I changed the output a bit to uses ints, arrays, etc. where it
>> seemed appropriate. The tests aren't quite done yet, but does
>> this new json output look good to you?
>>
>> Josh
>
> Hi,
>
> Sorry for the delay. Thanks for resolving the style issues and cleaning
> this up.
>
> Wrt the json output, I think it's fine as it is now.

Great!

> Just a minor comment. The --pretty-format option doesn't seem to be
> documented (in either rbd --help or rbd man page). Other than that, I
> don't see any other issues.

Thanks for pointing that out. I'll fix that, the tests, and then merge
it into master.

Josh