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 10:12:21 +0800 Message-ID: <53BF4805.1070900@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21438.52873.790154.902193@mariner.uk.xensource.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 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. > > 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.