[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
Hi, On Wednesday 17 June 2015 10:12:59 Cao jin wrote:
@@ -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.
I will send something to help with these issues, so no need to change this for now.
+ 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?
Possibly, but the user (as in caller for this API) still need to do some kind of parsing; given that you are basically copying bits from the btrfs output, they might change breaking users. Speaking of this: you said that you have a colleague working on btrfs-progs? What about suggesting to create some machine-parseable output (csv, xml, yaml, json, whatever) so extracting the results of btrfs tools is a lot more easy? Thanks, -- Pino Toscano
Hi, Pino 在 2015年06月17日 23:37, Pino Toscano 写道:
Hi,
On Wednesday 17 June 2015 10:12:59 Cao jin wrote:
@@ -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.
I will send something to help with these issues, so no need to change this for now.
OK
+ 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?
Possibly, but the user (as in caller for this API) still need to do some kind of parsing; given that you are basically copying bits from the btrfs output, they might change breaking users.
Speaking of this: you said that you have a colleague working on btrfs-progs? What about suggesting to create some machine-parseable output (csv, xml, yaml, json, whatever) so extracting the results of btrfs tools is a lot more easy?
Yes, I forward your suggestion and consult him, the result is not surprised:( Here is what I learned from him: For the btrfs-progs cmds who output strings, the output are plain, don`t have patterns. Seen some guys who want a formatted output, they do a filter by themself, it not reasonable to ask btrfs-progs to output formatted strings. What`s your opinion about the output pattern? I can try to implement it.
Thanks,
-- Yours Sincerely, Cao Jin
Hi, On Thursday 18 June 2015 11:01:37 Cao jin wrote:
Speaking of this: you said that you have a colleague working on btrfs-progs? What about suggesting to create some machine-parseable output (csv, xml, yaml, json, whatever) so extracting the results of btrfs tools is a lot more easy?
Yes, I forward your suggestion and consult him, the result is not surprised:( Here is what I learned from him: For the btrfs-progs cmds who output strings, the output are plain, don`t have patterns. Seen some guys who want a formatted output, they do a filter by themself, it not reasonable to ask btrfs-progs to output formatted strings.
This is exactly the issue here: every btrfs command has a totally different formatting for its output. Just let him take a look at all the commands implemented so far in libguestfs, you can easily spot that: - btrfs subvolume list - btrfs subvolume show - btrfs qgroup show - btrfs balance status - btrfs scrub status - btrfs device stats all have different custom parsers for each of their outputs. Compare that to e.g. the -m parameter for parted, so it outputs fields separated by semi-colon. -- Pino Toscano
Hi, Pino 在 2015年06月18日 16:41, Pino Toscano 写道:
Hi,
On Thursday 18 June 2015 11:01:37 Cao jin wrote:
Speaking of this: you said that you have a colleague working on btrfs-progs? What about suggesting to create some machine-parseable output (csv, xml, yaml, json, whatever) so extracting the results of btrfs tools is a lot more easy?
Yes, I forward your suggestion and consult him, the result is not surprised:( Here is what I learned from him: For the btrfs-progs cmds who output strings, the output are plain, don`t have patterns. Seen some guys who want a formatted output, they do a filter by themself, it not reasonable to ask btrfs-progs to output formatted strings.
This is exactly the issue here: every btrfs command has a totally different formatting for its output. Just let him take a look at all the commands implemented so far in libguestfs, you can easily spot that: - btrfs subvolume list - btrfs subvolume show - btrfs qgroup show - btrfs balance status - btrfs scrub status - btrfs device stats all have different custom parsers for each of their outputs.
Compare that to e.g. the -m parameter for parted, so it outputs fields separated by semi-colon.
Already forward your mail to my colleague. He think your comment is interesting, and also forward your option to btrfs-progs mail-list. Speaking of btrfs device stats output, what do you suggest to do for now? Since it may has multi devices. -- Yours Sincerely, Cao Jin
On Thursday 18 June 2015 18:33:59 Cao jin wrote:
Hi, Pino
在 2015年06月18日 16:41, Pino Toscano 写道:
Hi,
On Thursday 18 June 2015 11:01:37 Cao jin wrote:
Speaking of this: you said that you have a colleague working on btrfs-progs? What about suggesting to create some machine-parseable output (csv, xml, yaml, json, whatever) so extracting the results of btrfs tools is a lot more easy?
Yes, I forward your suggestion and consult him, the result is not surprised:( Here is what I learned from him: For the btrfs-progs cmds who output strings, the output are plain, don`t have patterns. Seen some guys who want a formatted output, they do a filter by themself, it not reasonable to ask btrfs-progs to output formatted strings.
This is exactly the issue here: every btrfs command has a totally different formatting for its output. Just let him take a look at all the commands implemented so far in libguestfs, you can easily spot that: - btrfs subvolume list - btrfs subvolume show - btrfs qgroup show - btrfs balance status - btrfs scrub status - btrfs device stats all have different custom parsers for each of their outputs.
Compare that to e.g. the -m parameter for parted, so it outputs fields separated by semi-colon.
Already forward your mail to my colleague. He think your comment is interesting, and also forward your option to btrfs-progs mail-list.
Great to hear that, thanks for it.
Speaking of btrfs device stats output, what do you suggest to do for now? Since it may has multi devices.
I see few options: (a) keep the current RHashTable (b) turn the statistics into a proper struct, and use RStructList (c) implement RHashTableList, and use it (d) something else I'm missing -- Pino Toscano
Hi 在 2015年06月18日 18:53, Pino Toscano 写道:
On Thursday 18 June 2015 18:33:59 Cao jin wrote:
Hi, Pino
在 2015年06月18日 16:41, Pino Toscano 写道:
Hi,
On Thursday 18 June 2015 11:01:37 Cao jin wrote:
Speaking of this: you said that you have a colleague working on btrfs-progs? What about suggesting to create some machine-parseable output (csv, xml, yaml, json, whatever) so extracting the results of btrfs tools is a lot more easy?
Yes, I forward your suggestion and consult him, the result is not surprised:( Here is what I learned from him: For the btrfs-progs cmds who output strings, the output are plain, don`t have patterns. Seen some guys who want a formatted output, they do a filter by themself, it not reasonable to ask btrfs-progs to output formatted strings.
This is exactly the issue here: every btrfs command has a totally different formatting for its output. Just let him take a look at all the commands implemented so far in libguestfs, you can easily spot that: - btrfs subvolume list - btrfs subvolume show - btrfs qgroup show - btrfs balance status - btrfs scrub status - btrfs device stats all have different custom parsers for each of their outputs.
Compare that to e.g. the -m parameter for parted, so it outputs fields separated by semi-colon.
Already forward your mail to my colleague. He think your comment is interesting, and also forward your option to btrfs-progs mail-list.
Great to hear that, thanks for it.
Speaking of btrfs device stats output, what do you suggest to do for now? Since it may has multi devices.
I see few options:
(a) keep the current RHashTable
(b) turn the statistics into a proper struct, and use RStructList
(c) implement RHashTableList, and use it
(d) something else I'm missing
OK, I prefer (a), its implementation seems easier and more clear than others. I will prepare a new patch, with commit message improved as you said -- Yours Sincerely, Cao Jin
On Thu, Jun 18, 2015 at 10:41:56AM +0200, Pino Toscano wrote:
Hi,
On Thursday 18 June 2015 11:01:37 Cao jin wrote:
Speaking of this: you said that you have a colleague working on btrfs-progs? What about suggesting to create some machine-parseable output (csv, xml, yaml, json, whatever) so extracting the results of btrfs tools is a lot more easy?
Yes, I forward your suggestion and consult him, the result is not surprised:( Here is what I learned from him: For the btrfs-progs cmds who output strings, the output are plain, don`t have patterns. Seen some guys who want a formatted output, they do a filter by themself, it not reasonable to ask btrfs-progs to output formatted strings.
This is exactly the issue here: every btrfs command has a totally different formatting for its output. Just let him take a look at all the commands implemented so far in libguestfs, you can easily spot that: - btrfs subvolume list - btrfs subvolume show - btrfs qgroup show - btrfs balance status - btrfs scrub status - btrfs device stats all have different custom parsers for each of their outputs.
Compare that to e.g. the -m parameter for parted, so it outputs fields separated by semi-colon.
I have to agree with Pino T that btrfs-progs would be a lot better if it had parseable output, like json/XML/etc, especially parseable output that was stable over time. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
participants (3)
-
Cao jin -
Pino Toscano -
Richard W.M. Jones