Libguestfs - Jun 2015 - Re: [PATCH] New API: btrfs_device_stats

Also modified a public function: analyze_line, make it more flexible

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
 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') {
@@ -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);
+  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)
+  {
+    if (add_string (&ret, key) == -1)
+      return NULL;
+
+    if (add_string (&ret, value) == -1)
+      return NULL;
+
+    p = analyze_line(p, &key, &value, ' ');
+  }
+
+  if (end_stringsbuf (&ret) == -1)
+    return NULL;
+
+  return ret.argv;
+}
+
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." };
+
 ]
 
 (* Non-API meta-commands available only in guestfish.
-- 
2.1.0

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@cn.fujitsu.com>
> ---
>  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,
-- 
Pino Toscano

Hi，

在 2015年06月16日 20:56, Pino Toscano 写道:
> 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?
>
OK
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>>   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.
I thought it is a good coding rule.
OK, will remove the local
>> @@ -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".
>
yup...will fix this. see some other APIs have the same problem.
>> +  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...
Ok, respect this rule. But personally speaking, I think it is easier for 
code reader to tell which two Curly brackets are a pair:)
>> +    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
> ?
In the condition that the btrfs have multi devices, its original output 
is going to this way:
   [/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
   [/dev/sdb].write_io_errs   0
   [/dev/sdb].read_io_errs    0
   [/dev/sdb].flush_io_errs   0
   [/dev/sdb].corruption_errs 0
   [/dev/sdb].generation_errs 0
   [/dev/sdc]...
   [/dev/sdc]...
   [/dev/sdc]...
   [/dev/sdc]...
   ...
So. I think the [/dev/sd..] is necessary, how to think?
>> 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.
>
OK, will remove the zero parameter
> Thanks,
>
-- 
Yours Sincerely,

Cao Jin