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

[xiezhide]

-----Original Message-----
From: Eric Blake [mailto:address@hidden 
Sent: 2018年11月16日 4:56
To: xiezhide <address@hidden>; address@hidden
Cc: address@hidden; address@hidden; address@hidden; address@hidden; zengcanfu 
00215970 <address@hidden>; Jinxuefeng <address@hidden>; Chenhui (Felix, Euler) 
<address@hidden>
Subject: Re: [PATCH v4 1/4] fsdev-throttle-qmp: factor out throttle code to 
reuse code

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.

Very helpful, thanks

> 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 <address@hidden>
> ---
>   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,

-----Fix it now

Thanks
Kidd