On Fri, Nov 21, 2014 at 05:17:13PM +0100, Pino Toscano wrote:
On Friday 21 November 2014 13:18:00 Hu Tao wrote:
> btrfs_subvolume_show shows the detailed information of a subvolume or
> snapshot.
>
> Signed-off-by: Hu Tao <hutao(a)cn.fujitsu.com>
> ---
> daemon/btrfs.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++
> generator/actions.ml | 9 +++
> src/MAX_PROC_NR | 2 +-
> 3 files changed, 177 insertions(+), 1 deletion(-)
>
> diff --git a/daemon/btrfs.c b/daemon/btrfs.c
> index e179b57..36ba588 100644
> --- a/daemon/btrfs.c
> +++ b/daemon/btrfs.c
> @@ -29,6 +29,7 @@
> #include "actions.h"
> #include "optgroups.h"
> #include "xstrtol.h"
> +#include "c-ctype.h"
>
> GUESTFSD_EXT_CMD(str_btrfs, btrfs);
> GUESTFSD_EXT_CMD(str_btrfstune, btrfstune);
> @@ -813,3 +814,169 @@ do_btrfs_fsck (const char *device, int64_t superblock, int
repair)
>
> return 0;
> }
> +
> +char **do_btrfs_subvolume_show (const char *subvolume)
> +{
> + const size_t MAX_ARGS = 64;
> + const char *argv[MAX_ARGS];
> + size_t i = 0;
> + CLEANUP_FREE char *subvolume_buf = NULL;
> + CLEANUP_FREE char *err = NULL;
> + CLEANUP_FREE char *out = NULL;
> + char *p, *pend, *colon;
> + DECLARE_STRINGSBUF (ret);
> + int r;
> +
> + subvolume_buf = sysroot_path (subvolume);
> + if (subvolume_buf == NULL) {
> + reply_with_perror ("malloc");
> + return NULL;
> + }
> +
> + ADD_ARG (argv, i, str_btrfs);
> + ADD_ARG (argv, i, "subvolume");
> + ADD_ARG (argv, i, "show");
> + ADD_ARG (argv, i, subvolume_buf);
> + ADD_ARG (argv, i, NULL);
> +
> + r = commandv (&out, &err, argv);
> + if (r == -1) {
> + reply_with_error ("%s: %s", subvolume, err);
> + return NULL;
> + }
> +
> + /* Output is:
> + *
> + * /
> + * Name: root
> + * uuid: c875169e-cf4e-a04d-9959-b667dec36234
> + * Parent uuid: -
> + * Creation time: 2014-11-13 10:13:08
> + * Object ID: 256
> + * Generation (Gen): 6579
> + * Gen at creation: 5
> + * Parent: 5
> + * Top Level: 5
> + * Flags: -
> + * Snapshot(s):
> + * snapshots/test1
> + * snapshots/test2
> + * snapshots/test3
> + *
> + */
> +
> + p = out;
> +
> + p = strchr (p, '\n');
> + if (p) {
> + *p = '\0';
> + p++;
> + }
> + else {
Minor style note:
} else {
(also in other parts of this patch)
Though I'm fine with it, but do we have coding style rules? I see both
styles in existing codes.
> + reply_with_error ("truncated output");
I'd print out anyway, to ease debugging failures a bit.
Okay.
> + return NULL;
> + }
> +
> + /* If the path is the btrfs root, `btrfs subvolume show' reports:
> + * <path> is btrfs root
> + */
> + if (strstr(out, "is btrfs root") != NULL) {
Take care of adding a space before parenthesis (also in other parts of
this patch).
Okay.
> + reply_with_error ("%s is btrfs root", subvolume);
> + return NULL;
> + }
> +
> + /* The first line is the path of the subvolume. */
> + if (add_string (&ret, strdup("path")) == -1) {
add_string duplicates the string, so you don't need to do that
manually.
Okay.
> + return NULL;
> + }
> + if (add_string (&ret, out) == -1) {
> + return NULL;
> + }
> +
> + /* Read the lines and split into "key: value". */
> + while (*p) {
> + /* leading spaces and tabs */
> + while (*p && c_isspace (*p)) p++;
Properly indent such lines, so:
while (*p && c_isspace (*p))
p++;
(also, a minor optimization is to use ++p instead of p++, as it can
avoid a temporary value)
Okay.
> +
> + pend = strchrnul (p, '\n');
> + if (*pend == '\n') {
> + *pend = '\0';
> + pend++;
> + }
> +
> + if (!*p) continue;
> +
> + colon = strchr (p, ':');
> + if (colon) {
> + *colon = '\0';
> +
> + /* snapshot is special, see the output above */
> + if (strncmp(p, "Snapshot", sizeof("Snapshot") - 1) == 0)
{
Please use the STR* macros we have in src/guestfs-internal-all.h.
Also, most probably it is better to match "Snapshot(s)", so if in the
future a new field starting with "Snapshot" is added then it won't be
mistaken for this multiline one.
Sounds good.
> + char *ss = NULL;
> +
> + if (add_string (&ret, p) == -1) {
> + return NULL;
> + }
Single statements don't need curly brackets (also in other parts of
the patch).
I think it is better to document this one, too.
> +
> + /* each line is the path of a snapshot. */
> + p = pend;
> + while (*p && strchr(p, ':') == NULL) {
This will break once the output will have fields after Snapshot, as
strchr(p, ':') will return a non-null value in one of the lines
following the first line after the Snapshot one.
It is intended, so that we can parse properly possibly key:value pairs
following "Snapshot(s)". Although a ':' in a snapshot path will hit it
too, but people don't likely put a ':' in path.
> + while (*p && c_isspace (*p)) p++;
> + pend = strchrnul (p, '\n');
> + if (*pend == '\n') {
> + *pend = '\0';
> + pend++;
> + }
I see this code repeated already; I think it would be better to create
a small helper function like:
char *analyze_line (char *p, char **field, char **value)
which would
- destructively update p (setting null bytes, just like it is done now)
- return in field the start of the field text
- return in value, if present, the start of the value (after the colon)
- return the pointer to the first character of the new line (or null,
if none following)
this way it is IMHO easier to iterate over the lines, and also to detect
whether a line contains a field+value or just text.
How to treat lines don't containing a colon? treat them as invalid line?
or as value of key in previous line? Note this should be a general
process.
> + if (ss) {
> + ss = realloc(ss, strlen(ss) + strlen(p));
If realloc fails, the return value is null; this means that the
following will leak ss in that case.
Also, sounds like you are missing "," in the length of the string?
> + strcat(ss, ",");
Just save the length of ss before the realloc, and you can simply do
ss[old_length_of_ss] = ',';
> + strcat(ss, p);
You can easily use memcpy here:
memcpy (ss + old_length_of_ss + 1, p, strlen (p));
> + } else {
> + ss = strdup(p);
Missing check for failed strdup.
> + }
> +
> + p = pend;
> + }
> +
> + if (ss) {
> + if (add_string (&ret, ss) == -1) {
This needs to be add_string_nodup.
> + free(ss);
No need to free ss here; theoretically add_string will take care of it,
be it added to the stringbuf or freed on error.
This is fail case.
> + return NULL;
> + }
> + }
> + else {
> + if (add_string (&ret, strdup("")) == -1) {
> + return NULL;
> + }
> + }
If there are no elements for "Snapshot(s)", then it would be better
to just not add an empty hash element.
> +
> + continue;
> + }
> +
> + do { colon++; } while (*colon && c_isspace (*colon));
> + if (add_string (&ret, p) == -1) {
> + return NULL;
> + }
> + if (add_string (&ret, colon) == -1) {
> + return NULL;
> + }
At least from the example output added as comment, it seems that
"not available" values might be "-" -- it'd just not add those,
since
as users of this API would still need to check for the existency of
a certain key in the returned hash.
tune2fs-l lists keys with empty values, shouldn't we follow it?
> + }
> + else {
> + if (add_string (&ret, p) == -1) {
> + return NULL;
> + }
> + if (add_string (&ret, "") == -1) {
> + return NULL;
> + }
As above, I would not add elements with empty values in the hash.
> + }
> +
> + p = pend;
> + }
> +
> + if (end_stringsbuf (&ret) == -1)
> + return NULL;
> +
> + return ret.argv;
> +
> +}
> diff --git a/generator/actions.ml b/generator/actions.ml
> index cf96039..24d3ecc 100644
> --- a/generator/actions.ml
> +++ b/generator/actions.ml
> @@ -12014,6 +12014,15 @@ This is the internal call which implements
C<guestfs_lstatnslist>." };
> longdesc = "\
> Get the default subvolume or snapshot of a filesystem mounted at
C<mountpoint>." };
>
> + { defaults with
> + name = "btrfs_subvolume_show";
> + style = RHashtable "btrfssubvolumeinfo", [Pathname
"subvolume"], [];
> + proc_nr = Some 425;
> + optional = Some "btrfs"; camel_name =
"BTRFSSubvolumeShow";
> + shortdesc = "show detailed information of the subvolume";
"returns detailed information about the subvolume"
> + longdesc = "\
> +show detailed information of the subvolume." };
Like above, making sure it is properly capitalized at the beginning.
sure.
Thank you for reviewing!
--
Pino Toscano