Re: [PATCH v12 26/26] cmdline switches and config vars to control colo-proxy

From: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Lars Kurth <lars.kurth@citrix.com>,
	Li Zhijian <lizhijian@cn.fujitsu.com>,
	Wei Liu <wei.liu2@citrix.com>,
	Ian Campbell <ian.campbell@citrix.com>,
	Wen Congyang <wency@cn.fujitsu.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Jiang Yunhong <yunhong.jiang@intel.com>,
	Dong Eddie <eddie.dong@intel.com>,
	xen devel <xen-devel@lists.xen.org>,
	Anthony Perard <anthony.perard@citrix.com>,
	Gui Jianfeng <guijianfeng@cn.fujitsu.com>,
	Shriram Rajagopalan <rshriram@cs.ubc.ca>,
	Yang Hongyang <hongyang.yang@easystack.cn>
Subject: Re: [PATCH v12 26/26] cmdline switches and config vars to control colo-proxy
Date: Fri, 25 Mar 2016 10:57:41 +0800	[thread overview]
Message-ID: <56F4A925.5010602@cn.fujitsu.com> (raw)
In-Reply-To: <22260.4598.661909.20352@mariner.uk.xensource.com>

On 03/25/2016 12:12 AM, Ian Jackson wrote:
> Changlong Xie writes ("[PATCH v12 26/26] cmdline switches and config vars to control colo-proxy"):
>> From: Wen Congyang <wency@cn.fujitsu.com>
>>
>> Add cmdline switches to 'xl migrate-receive' command to specify
>> a domain-specific hotplug script to setup COLO proxy.
>
>> +    if (nic->forwarddev) {
>> +        flexarray_append(back, "forwarddev");
>> +        flexarray_append(back, nic->forwarddev);
>> +    }
>
> I'd prefer a name with `coloft' in it, throughout, even for the
> xenstore node.  But again this is not critical if we are not trying to
> make a stable API.
>
>> +    static struct option opts[] = {
>> +        {"script", 1, 0, 0x100},
>
> This should surely be --coloft-script, or --colo-script, something.
>

I prefer "--coloft-script"

> I think this script is (actually, in your setup) created by your
> management software, and contains some actual functionality.  Am I
> right ?  In which case my comments about including that functionality

Yes

> in xen.git apply here too.  But again this is not a blocker for 4.7.
>
>> -            xasprintf(&rune, "exec %s %s xl migrate-receive %s %s",
>> -                      ssh_command, host,
>> -                      libxl_defbool_val(r_info.colo) ? "-c" : "-r",
>> -                      daemonize ? "" : " -e");
>> +            if (!libxl_defbool_val(r_info.colo)) {
>> +                xasprintf(&rune, "exec %s %s xl migrate-receive %s %s",
>> +                          ssh_command, host,
>> +                          "-r",
>> +                          daemonize ? "" : " -e");
>> +            } else {
>> +                xasprintf(&rune, "exec %s %s xl migrate-receive %s %s %s %s",
>> +                          ssh_command, host,
>> +                          "-c",
>> +                          r_info.netbufscript ? "--script" : "",
>> +                          r_info.netbufscript ? r_info.netbufscript : "",
>> +                          daemonize ? "" : " -e");
>
> I have just noticed here that you have introduced `-c' (in an earlier
> patch) to mean to receive a COLO checkpointed stream.
>
> Can you please change this to `--colo' ?
>

Will replace "-c" with "--colo"

Thanks
	-Xie

> Sorry,
> Ian.
>
>
> .
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel