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

[Josh Durgin <josh.durgin@inktank.com>]

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