-----Original Message----- From: Eric Blake [mailto:ebl...@redhat.com] Sent: 2018年11月16日 4:56 To: xiezhide <xiezh...@huawei.com>; qemu-devel@nongnu.org Cc: gr...@kaod.org; aneesh.ku...@linux.vnet.ibm.com; arm...@redhat.com; be...@igalia.com; zengcanfu 00215970 <zengca...@huawei.com>; Jinxuefeng <jinxuef...@huawei.com>; Chenhui (Felix, Euler) <chenhui.r...@huawei.com> 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 <xiezh...@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, -----Fix it now Thanks Kidd
