[Libguestfs] [PATCH 6/6] New API: btrfs_subvolume_show
Hu Tao
hutao at cn.fujitsu.com
Wed Nov 26 02:49:17 UTC 2014
On Mon, Nov 24, 2014 at 06:17:22PM +0100, Pino Toscano wrote:
> On Monday 24 November 2014 16:22:53 Hu Tao wrote:
> > 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.
>
> >From what I see, «} else {» is more common.
Okay.
>
> > > > +
> > > > + /* 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.
>
> Well, that's not what would happen: "p" points to the first character of the line after the "Snapshot(s)" one, and strchr() will try to find ':' until the end of the string, not stopping at newlines and such.
>
> Let's say the output is something like:
>
> 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
> Other attribute: -
>
> p would be at:
> snapshots/test1
> ^- here, while strchr(p, ':') == NULL) would be
> Other attribute: -
> ^- here, so skipping all the multi-line value.
You're right:) fixed.
>
> > > > + 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.
>
> field pointing at their non-blank start, and value as null.
I am now realizing I asked a question not related to analyze_line().
(I am trying to elaborate it below but not rasing a question)
Say we have these lines:
key1:
value1
value2
value3
key2 <--- (this is a malformed string lacks : and value)
for each lines, we will get following results:
field: key1 value: ""
field: value1 value: (null)
field: value2 value: (null)
field: value3 value: (null)
field: key2 value: (null)
for "value1"-"value3", we can append them to value of key1, because this is
what we want. But for "key2", we append it as value too. In another
word, we don't have the ability to detect malformed lines.
However, this is what I've done in my patch.
>
> > > > + 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.
>
> I don't understand what you mean here, can you please be more verbose?
The code is:
if (add_string (&ret, ss) == -1) {
free (ss);
return NULL;
}
So the ss is not yet added to stringbug, I think in the case ss should
be freed manually. But if you mean ss will be freed later even in this
case (I'm not sure about it), then we can remove the free line safely.
>
> > > > +
> > > > + 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?
>
> Do you have an example of them? I haven't used tune2fs-l much myself,
> but I don't remember having seen empty values in its output.
See below, tune2fs-l on a freshly mkfs-ed ext3 filesystem shows some blank fields:
><fs> mkfs ext3 /dev/sda6
><fs> tune2fs-l /dev/sda6
Filesystem volume name:
Last mounted on:
Filesystem UUID: e4ee511c-c8c6-4e6a-baee-9b7e1375b296
Filesystem magic number: 0xEF53
Filesystem revision #: 1 (dynamic)
Filesystem features: ext_attr resize_inode dir_index filetype sparse_super
Filesystem flags: signed_directory_hash
Default mount options: user_xattr acl
Filesystem state: clean
Errors behavior: Continue
Filesystem OS type: Linux
Inode count: 128
Block count: 1020
Reserved block count: 51
Free blocks: 982
Free inodes: 117
First block: 1
Block size: 1024
Fragment size: 1024
Reserved GDT blocks: 3
Blocks per group: 8192
Fragments per group: 8192
Inodes per group: 128
Inode blocks per group: 16
Filesystem created: Wed Nov 26 02:25:19 2014
Last mount time: n/a
Last write time: Wed Nov 26 02:25:19 2014
Mount count: 0
Maximum mount count: -1
Last checked: Wed Nov 26 02:25:19 2014
Check interval: 0 (<none>)
Reserved blocks uid: 0 (user root)
Reserved blocks gid: 0 (group root)
First inode: 11
Inode size: 128
Default directory hash: half_md4
Directory Hash Seed: dd7fbb80-1356-47c8-b99e-cbec6970df15
>
> --
> Pino Toscano
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
More information about the Libguestfs
mailing list