Re: [libvirt] [PATCH 5/6 v3] remote-protocol: Remote protocol implementation of virDomainSetBlkioParameters and virDomainGetBlkioParameters.

On 02/21/2011 10:34 PM, Gui Jianfeng wrote: > Remote protocol implementation of virDomainSetBlkioParameters and virDomainGetBlkioParameters. > > Signed-off-by: Gui Jianfeng <guijianfeng(a)cn.fujitsu.com&gt; > --- > daemon/remote.c | 210 +++++++++++++++++++++++++++++++++++ > daemon/remote_dispatch_args.h | 2 + > daemon/remote_dispatch_prototypes.h | 16 +++ > daemon/remote_dispatch_ret.h | 1 + > daemon/remote_dispatch_table.h | 10 ++ > src/remote/remote_driver.c | 173 ++++++++++++++++++++++++++++- > src/remote/remote_protocol.c | 89 +++++++++++++++ > src/remote/remote_protocol.h | 55 +++++++++ > src/remote/remote_protocol.x | 44 +++++++- > src/remote_protocol-structs | 35 ++++++ > 10 files changed, 632 insertions(+), 3 deletions(-) ACK, although I had quite the rough time applying this after the conflicts with virDomainSetMemoryFlags. I ended up shuffling things to match shuffles earlier in the series. > +++ b/src/remote/remote_protocol.x > @@ -128,6 +128,9 @@ const REMOTE_NWFILTER_NAME_LIST_MAX = 1024; > /* Upper limit on list of scheduler parameters. */ > const REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX = 16; > > +/* Upper limit on list of blkio parameters. */ > +const REMOTE_DOMAIN_blkio_PARAMETERS_MAX = 16; > + Wrong case. How'd you test this? > @@ -2103,7 +2143,9 @@ enum remote_procedure { > > REMOTE_PROC_DOMAIN_OPEN_CONSOLE = 201, > REMOTE_PROC_DOMAIN_IS_UPDATED = 202, > - REMOTE_PROC_GET_SYSINFO = 203 > + REMOTE_PROC_GET_SYSINFO = 203, > + REMOTE_PROC_DOMAIN_SET_BLKIO_PARAMETERS = 204, > + REMOTE_PROC_DOMAIN_GET_BLKIO_PARAMETERS = 205 SET_MEMORY_FLAGS beat you to 204; you get 205 and 206. Hmm, I guess that completes my review, so I'm pushing it. Thanks again for the work! However, I do have a parting comment that this was a _lot_ of code duplication; if we ever add another cgroup tunable, it would be worth the time to first factor out the common elements into something that memtune, blkio, and that new cgroup tunable could share. For example, we don't need three copies of the union for i/ui/l/ul/d/b - that should be something easy to share between tunables.