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

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,