From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hongyang Yang Subject: Re: [PATCH v15 2/7] remus: introduce remus device Date: Fri, 11 Jul 2014 14:16:49 +0800 Message-ID: <53BF8151.1050402@cn.fujitsu.com> References: <1404721584-17862-1-git-send-email-yanghy@cn.fujitsu.com> <1404721584-17862-3-git-send-email-yanghy@cn.fujitsu.com> <21437.31542.967350.687109@mariner.uk.xensource.com> <53BDF80F.7030307@cn.fujitsu.com> <21438.52873.790154.902193@mariner.uk.xensource.com> <53BF4805.1070900@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53BF4805.1070900@cn.fujitsu.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: laijs@cn.fujitsu.com, wency@cn.fujitsu.com, andrew.cooper3@citrix.com, yunhong.jiang@intel.com, eddie.dong@intel.com, xen-devel@lists.xen.org, rshriram@cs.ubc.ca, ian.campbell@citrix.com List-Id: xen-devel@lists.xenproject.org On 07/11/2014 10:12 AM, Hongyang Yang wrote: > > > On 07/11/2014 01:34 AM, Ian Jackson wrote: >> Hongyang Yang writes ("Re: [PATCH v15 2/7] remus: introduce remus device"): >>> On 07/10/2014 01:26 AM, Ian Jackson wrote: >>>> Sorry to mention this now, but I think it would be clearer if all >>>> these libxl__remus_device_* functions which manipulate _all_ the >>>> devices for a domain used the plural of "device", ie >>>> libxl__remus_devices_setup, etc. >>> >>> Ok >> >> Thanks. You may find git-filter-branch helpful to do this easily. >> (Be careful not to blow your leg off.) > > Thanks for the tip. > >> >>>>> + rds->num_devices++; >>>>> + /* >>>>> + * we add devices that have been setuped to the array no matter >>>>> + * the setup process succeed or failed because we need to ensure >>>>> + * the device been teardown while setup failed. If any of the >>>>> + * device setup failed, we will quit remus, but before we exit, >>>>> + * we will teardown the devices that have been added to **dev >>>>> + */ >>>>> + rds->dev[rds->num_set_up++] = dev; >>>>> + if (rc) { >>>>> + /* setup failed */ >>>>> + rs->saved_rc = ERROR_FAIL; >>>>> + } >>>> >>>> This doesn't look right. If the setup fails, presumably we shouldn't >>>> put the device in the array ? If we do it will presumably be torn >>>> down later. >>> >>> the netbuf script was designed as below: >>> 1. when setup failed, the script won't teardown the device itself. >>> 2. the teardown op is ok to be excute many times. >> >> Aha. Right. >> >> I think you should state exactly that, probably in a comment here and >> also in the script itself. This can replace the comment you have >> above, which is rather vague. >> >>> In the remus device layer, if one device setup failed(whether script >>> exit or the script get killed or something), we will quit >>> remus, but before that, we will teardown all device that has been set >>> up, whether it's correctly set up or not. So if we don't put the >>> device in the array, we will get a leaked device, that has not been >>> teardown. >> >> That makes sense. >> >>>> I still don't understand why libxl__remus_device_state is not part of >>>> libxl__remus_state. >>> >>> libxl__remus_device_state is part of libxl__remus_state: >>> +struct libxl__remus_state { >> ... >>> + libxl__remus_device_state dev_state; >>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> >> I meant: why is it a separate structure, rather than the contents >> simply being included there ? >> > > Err, I thought I've replied on the last thread about this, but I will > reply here. > I intend to use libxl__remus_state on upper layer, that is, in the > main flow of libxl layer, and use libxl__remus_device_state in both > remus abstract layer and concrete layer. I thought that will make > things more clear. But yes, there still some libxl__remus_state use > in remus abstract layer and concrete layer, I will fix it up in the > next version. I reconsidered about this case, most part of libxl__remus_state can be merged into libxl__remus_device_state, there's only one left: libxl__ev_time timeout; /* used for checkpoint */ but I think this can be moved to libxl__domain_suspend_state just like interval for remus: struct libxl__domain_suspend_state { ...snip... const char *dm_savefile; int interval; /* checkpoint interval (for Remus) */ + libxl__ev_time checkpoint_timeout; /* used for remus checkpoint */ ...snip... }; So, I'll merge libxl__remus_state into libxl__remus_device_state. > >> >> Thanks for your other replies. You don't seem to have replied to >> everything I said, including some questions I asked. Do you intend >> to, later, perhaps ? > > Sorry, I might have missed some of your comments or questions, but > that's not what I was intend to...I was trying to reply every question > you've pointed out. I will go back and go through your comments carefully. > >> >> Thanks, >> Ian. >> . >> > -- Thanks, Yang.