From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58555) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7DFl-0007k8-7y for qemu-devel@nongnu.org; Mon, 22 Jun 2015 21:43:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z7DFi-0006XJ-1G for qemu-devel@nongnu.org; Mon, 22 Jun 2015 21:43:57 -0400 Received: from szxga03-in.huawei.com ([119.145.14.66]:19931) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z7DFh-0006Ut-DI for qemu-devel@nongnu.org; Mon, 22 Jun 2015 21:43:53 -0400 Message-ID: <5588B9C8.9060800@huawei.com> Date: Tue, 23 Jun 2015 09:43:36 +0800 From: zhanghailiang MIME-Version: 1.0 References: <20150605095004.GB2139@work-vm> <55815000.8060300@huawei.com> <87d20st8vt.fsf@blackfin.pond.sub.org> In-Reply-To: <87d20st8vt.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] Adding new migration-parameters - any easier way? List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: quintela@redhat.com, qemu-devel@nongnu.org, peter.huangpeng@huawei.com, amit.shah@redhat.com, "Dr. David Alan Gilbert" On 2015/6/19 16:11, Markus Armbruster wrote: > zhanghailiang writes: > >> Hi, >> >> Is there any news about this discussion? >> Is anyone working on it? ;) >> >> Since the 'hard feature freeze' time is closer, we'd better to fix it in 2.4 >> before libvirt uses it. >> >> I have sent a RFC patch "[RFC] migration: Re-implement 'migrate-set-parameters' to make it easily for extension" >> http://patchwork.ozlabs.org/patch/482125/ >> Which just changes qmp command to >> '{ "execute": "migrate-set-parameters" , "arguments": >> { "compression": { "compress-level": 1 } } }', >> Compared with its original way, it seems to be more easy for reading and extension, but >> it can't address most problems discussed here... > >>>From a QMP wire protocol point of view, @migrate-set-parameters is just > fine as it is: > > # > # @migrate-set-parameters > # > # Set the following migration parameters > # > # @compress-level: compression level > # > # @compress-threads: compression thread count > # > # @decompress-threads: decompression thread count > # > # Since: 2.4 > ## > { 'command': 'migrate-set-parameters', > 'data': { '*compress-level': 'int', > '*compress-threads': 'int', > '*decompress-threads': 'int'} } > > However, as the number of parameters grows, the generated C interface > becomes more and more unwieldy: > > void qmp_migrate_set_parameters(bool has_compress_level, > int64_t compress_level, > bool has_compress_threads, > int64_t compress_threads, > bool has_decompress_threads, > int64_t decompress_threads, > ... more ... > Error **errp) > Yes, this is the problem. > Your [RFC PATCH] hides away some of the parameters in a separate > MigrationCompressParameter (misnamed, should be plural). Leads to > > void qmp_migrate_set_parameters(bool has_compress, > MigrationCompressParameters *compress, > ... more ... > Error **errp) > > Makes the QMP wire protocol more complex, because now you have to > collect some parameters in extra curlies. > > Note the two levels of optionalness: one for the wrapping struct, one > for each of its members. If you made the members non-optional, we'd > have to specify none or all, which is not what we want. If you made the > wrapping struct non-optional, you'd have to add a stupid 'compress': {} > when you want to set something else. So we need both. > > In C, the relatively simple test has_compress_level becomes compress && > compress->has_level. > Hmm, i see, thanks for your explanation. > If we ever want to set migration parameters from the command line, the > nesting will be in the way. Our tool to parse command line into > QAPI-generated data types (OptsVisitor) by design doesn't cope with > nested structs. > > Perhaps the extra complexity is worthwhile, but it's certainly not > obvious. > > If all we want is making adding new parameters easier (this thread's > stated subject), then I guess having the function take a single struct > parameter for all its arguments would do: > > void qmp_migrate_set_parameters(MigrationParameters *parms, > Error **errp) > > To get that from the current QAPI code generator, we'd have to do > something like > > { 'command': 'migrate-set-parameters', > 'data': { 'parms': 'MigrationParameters' } } > > Trouble is this messes up the QMP wire interface: you now have to wrap > the actual arguments in two curlies instead of one. > > Not a backward-compatibility issue, because the command is new. > > Ugly all the same. > > To avoid the ugliness, we could change the QAPI generator. Currently, > > { 'command': 'migrate-set-parameters', > 'data': 'MigrationParameters' } > > generates the same interface as when you inline MigrationParameters, > namely > > void qmp_migrate_set_parameters(bool has_compress_level, > int64_t compress_level, > bool has_compress_threads, > int64_t compress_threads, > bool has_decompress_threads, > int64_t decompress_threads, > ... more ... > Error **errp) > > It could instead generate > > void qmp_migrate_set_parameters(MigrationParameters *parms, > Error **errp) > > No change to the wire protocol. Fairly big, but relatively mechanical > change to the handler functions. I'd be willing to give it a shot and > see how it turns out, but I can't do it for 2.4, sorry. > Great, that would be fine~ Thanks.