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