Re: [Qemu-devel] [PATCH v4 1/4] fsdev-throttle-qmp: factor out throttle code to reuse code

[Eric Blake <eblake@redhat.com>]

On 11/15/18 2:54 AM, xiezhide wrote:
> This patch factor out throttle parameter parse code to common function

s/factor/factors/

Actually, when I write commit messages, I like to use the imperative 
mood, with an implicit "Apply this patch in order to" unspoken prefix. 
Starting with an explicit "This patch does" is more of a descriptive 
mood. So I might have written:

Factor out the throttle parameter parsing code to a new common function 
which will be used by block and fsdev.

> which will be used by block and fsdev.
> rename function throttle_parse_options to throttle_parse_group to resolve
> function name conflict
> 
> Signed-off-by: xiezhide <xiezhide@huawei.com>
> ---
>   block/throttle.c                |  6 ++--
>   blockdev.c                      | 43 +-------------------------
>   fsdev/qemu-fsdev-throttle.c     | 44 ++------------------------
>   include/qemu/throttle-options.h |  2 ++
>   include/qemu/throttle.h         |  4 +--
>   include/qemu/typedefs.h         |  1 +
>   util/throttle.c                 | 68 +++++++++++++++++++++++++++++++++++++++++
>   7 files changed, 79 insertions(+), 89 deletions(-)
> 
> +++ b/include/qemu/throttle-options.h
> @@ -111,4 +111,6 @@
>               .help = "when limiting by iops max size of an I/O in bytes",\
>           }
>   
> +void throttle_parse_options(ThrottleConfig *, QemuOpts *);

It's okay to use parameter names in function declarations.

> +++ b/include/qemu/typedefs.h
> @@ -113,6 +113,7 @@ typedef struct uWireSlave uWireSlave;
>   typedef struct VirtIODevice VirtIODevice;
>   typedef struct Visitor Visitor;
>   typedef struct node_info NodeInfo;
> +typedef struct ThrottleConfig ThrottleConfig;

Please keep this in the right sorted location, right after SSIBus.

[Hmm, we already have an inconsistency on whether we are sorting by 
en_US rules, which are case-insensitive and therefore has 'struct 
node_info' in the wrong location, or by C rules which sort lowercase 
later and therefore has 'struct uWireSlave' in the wrong position. For 
that matter, why are we renaming 'struct node_info NodeInfo', and why 
does uWireSlave violate our naming conventions?  But those are 
independent cleanups, and not necessarily something for you to worry about]

With the sorting fixed,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org