Re: [Libguestfs] [PATCH] New API: btrfs_device_stats

On Tuesday 16 June 2015 17:10:09 Cao jin wrote: > Also modified a public function: analyze_line, make it more flexible The addition of the parameter to analyze_line should be in an own patch. Can you please decouple it?

> Signed-off-by: Cao jin <caoj.fnst(a)cn.fujitsu.com&gt; > --- > daemon/btrfs.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++----- > generator/actions.ml | 12 ++++++++ > 2 files changed, 88 insertions(+), 7 deletions(-) > > diff --git a/daemon/btrfs.c b/daemon/btrfs.c > index 39392f7..f913f66 100644 > --- a/daemon/btrfs.c > +++ b/daemon/btrfs.c > @@ -853,11 +853,11 @@ do_btrfs_fsck (const char *device, int64_t superblock, int repair) > * returns the next position following \n. > */ > static char * > -analyze_line (char *line, char **key, char **value) > +analyze_line (char *line, char **key, char **value, char delim) > { > char *p = line; > char *next = NULL; > - char delimiter = ':'; > + char delimiter = delim; > char *del_pos = NULL; > > if (!line || *line == '\0') { Why keep the local variable 'delimiter'? Just name the argument like that.

> @@ -964,7 +964,7 @@ do_btrfs_subvolume_show (const char *subvolume) > * snapshots/test3 > * > */ > - p = analyze_line(out, &key, &value); > + p = analyze_line(out, &key, &value, ':'); > if (!p) { > reply_with_error ("truncated output: %s", out); > return NULL; > @@ -984,7 +984,7 @@ do_btrfs_subvolume_show (const char *subvolume) > } > > /* Read the lines and split into "key: value". */ > - p = analyze_line(p, &key, &value); > + p = analyze_line(p, &key, &value, ':'); > while (key) { > /* snapshot is special, see the output above */ > if (STREQLEN (key, "Snapshot(s)", sizeof ("Snapshot(s)") - 1)) { > @@ -994,7 +994,7 @@ do_btrfs_subvolume_show (const char *subvolume) > if (add_string (&ret, key) == -1) > return NULL; > > - p = analyze_line(p, &key, &value); > + p = analyze_line(p, &key, &value, ':'); > > while (key && !value) { > ss = realloc (ss, ss_len + strlen (key) + 1); > @@ -1008,7 +1008,7 @@ do_btrfs_subvolume_show (const char *subvolume) > ss_len += strlen (key); > ss[ss_len] = '\0'; > > - p = analyze_line(p, &key, &value); > + p = analyze_line(p, &key, &value, ':'); > } > > if (ss) { > @@ -1031,7 +1031,7 @@ do_btrfs_subvolume_show (const char *subvolume) > return NULL; > } > > - p = analyze_line(p, &key, &value); > + p = analyze_line(p, &key, &value, ':'); > } > } > > @@ -2083,3 +2083,72 @@ do_btrfs_image (char *const *sources, const char *image, > > return 0; > } > + > +char ** > +do_btrfs_device_stats (const char *path, int zero) > +{ > + const size_t MAX_ARGS = 64; > + const char *argv[MAX_ARGS]; > + size_t i = 0; > + CLEANUP_FREE char *buf = NULL; > + CLEANUP_FREE char *err = NULL; > + CLEANUP_FREE char *out = NULL; > + char *p, *key = NULL, *value = NULL; > + DECLARE_STRINGSBUF (ret); 'ret' is leaked if returning before "return ret.argv".

> + int r; > + int is_dev; > + > + is_dev = STREQLEN (path, "/dev/", 5); > + buf = is_dev ? strdup (path) : sysroot_path (path); > + if (buf == NULL) { > + reply_with_perror ("malloc"); > + return NULL; > + } > + > + > + ADD_ARG (argv, i, str_btrfs); > + ADD_ARG (argv, i, "device"); > + ADD_ARG (argv, i, "stats"); > + ADD_ARG (argv, i, buf); > + > + if ((optargs_bitmask & GUESTFS_BTRFS_DEVICE_STATS_ZERO_BITMASK) && zero) { > + ADD_ARG (argv, i, "-z"); > + } > + > + ADD_ARG (argv, i, NULL); > + > + r = commandv (&out, &err, argv); > + if (r == -1) { > + reply_with_error ("%s: %s", path, err); > + return NULL; > + } > + > + /* Output pattern is: > + * > + * [/dev/sda].write_io_errs 0 > + * [/dev/sda].read_io_errs 0 > + * [/dev/sda].flush_io_errs 0 > + * [/dev/sda].corruption_errs 0 > + * [/dev/sda].generation_errs 0 > + * > + */ > + > + /* Read the lines and split into "key: value". */ > + p = analyze_line(out, &key, &value, ' '); > + while(key) > + { Curly bracket goes to the line before, and please remember the space between function and opening round bracket...

> + if (add_string (&ret, key) == -1) > + return NULL; > + > + if (add_string (&ret, value) == -1) > + return NULL; > + > + p = analyze_line(p, &key, &value, ' '); > + } This means that the return "hash" will have keys like: [/dev/sda].write_io_errs ? Wouldn't it better to just return the name of the attribute, i.e. write_io_errs ?

> diff --git a/generator/actions.ml b/generator/actions.ml > index d5e5ccf..dfb42b5 100644 > --- a/generator/actions.ml > +++ b/generator/actions.ml > @@ -12593,6 +12593,18 @@ numbered C<partnum> on device C<device>. > > It returns C<primary>, C<logical>, or C<extended>." }; > > + { defaults with > + name = "btrfs_device_stats"; added = (1, 29, 46); > + style = RHashtable "devicestats", [Dev_or_Path "path"], [OBool "zero"]; > + proc_nr = Some 456; > + optional = Some "btrfs"; camel_name = "BTRFSDeviceStats"; > + shortdesc = "read and print the device IO stats for mounted btrfs device"; > + longdesc = "\ > +Read and print the device IO stats for all mounted devices of the filesystem > +identified by C<path>, or for a single device identified by C<path>. > + > +If you want to reset stats to zero after reading them, use option: zero:true." }; The 'zero' parameter adds side-effects to a "query" API, and this does not seem too good. If you want a way to reset this kind of btrfs metadata, then please add a separate / well distinct API for it. Just because btrfs subcommands have that awkward parameters, it doesn't mean we need to map them 1:1 in libguestfs.

Thanks,