From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Jackson Subject: Re: [PATCH 07/10 V7] libxl: use the API to setup/teardown network buffering [and 1 more messages] Date: Wed, 23 Apr 2014 17:02:53 +0100 Message-ID: <21335.58413.243916.380152@mariner.uk.xensource.com> References: <1397540297-32184-1-git-send-email-yanghy@cn.fujitsu.com> <1397540297-32184-6-git-send-email-yanghy@cn.fujitsu.com> <1392023972-24675-1-git-send-email-laijs@cn.fujitsu.com> <1392023972-24675-8-git-send-email-laijs@cn.fujitsu.com> <21268.49462.367220.516477@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <21268.49462.367220.516477@mariner.uk.xensource.com>, <1397540297-32184-6-git-send-email-yanghy@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: Yang Hongyang , Ian Jackson Cc: Ian Campbell , FNST-Wen Congyang , Stefano Stabellini , Andrew Cooper , Jiang Yunhong , Dong Eddie , Lai Jiangshan , Shriram Rajagopalan , xen-devel@lists.xen.org, ian.jackson@eu.citrix.com, Roger Pau Monne List-Id: xen-devel@lists.xenproject.org Yang Hongyang writes ("[PATCH V9 05/12] remus: remus device core and APIs to setup/teardown"): > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h > index 33b62a2..421ae24 100644 > --- a/tools/libxl/libxl_internal.h > +++ b/tools/libxl/libxl_internal.h > @@ -2457,8 +2457,35 @@ typedef struct libxl__logdirty_switch { > libxl__ev_time timeout; > } libxl__logdirty_switch; > > +typedef struct libxl__remus_state { > + libxl__domain_suspend_state *dss; > + libxl__egc *egc; > + > + /* private */ > + int saved_rc; > + /* Opaque context containing device related stuff */ > + void *device_state; > +} libxl__remus_state; I'm afraid that the interface between the remus code and the rest of the code is still not very clear. Earlier, I wrote: > [...] I wonder if it might not be better to provide a firmer > interface between the remus code and the rest of the save/restore > machinery. That is, have an explicit callback function recorded > by the save/restore code which is called back by the remus > machinery when it has done its work. What do you think ? > > I think having the flow of control spring off into libxl_remus.c and > magically come back by libxl_remus.c knowing to call > domain_suspend_done is rather opaque. I think you have basically two options: 1. Make the remus part of this be a fully self-contained standard asynchronous callback-based suboperation, like libxl__xswait, libxl__bootloader, et al. In this case you should rigorously follow the existing patterns, defining a clear interface between the two parts, providing a callback function set by the caller, etc. 2. Integrate the remus part into the suspend/resume code in an ad hoc fashion, with extremely clear comments everywhere about the expected interface, and no extraneous moving parts. At the moment you seem to have mixed these two approaches. > @@ -2470,6 +2497,7 @@ struct libxl__domain_suspend_state { > int live; > int debug; > const libxl_domain_remus_info *remus; > + libxl__remus_state *remus_state; I'm not sure why this variable is called "remus_state" rather than just "remus". > +typedef struct libxl__remus_device_state { > + /* nic */ > + libxl_device_nic *nics; > + int num_nics; > + > + /* disk */ > + libxl_device_disk *disks; > + int num_disks; Much of this doesn't seem be used in this patch. I think you may need to restructure your patch series. In general, when patching existing code, you should introduce an internal structure or variable in the same patch as you introduce the code which uses it. This is in contrast to new functions (or other facilities) with a well-defined API, where it is usually best to introduce the function fully-formed and then the callers. Since I normally look at the header file first, to try to see what all the pieces are and what's going on, it's difficult for me to review this patch in more detail as it stands. Thanks, Ian.