[Libguestfs] [PATCH 6/6] New API: btrfs_subvolume_show
Hu Tao
hutao at cn.fujitsu.com
Mon Nov 24 08:22:53 UTC 2014
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 at 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
More information about the Libguestfs
mailing list