Re: [Xen-devel] [PATCH 07/10 V7] libxl: use the API to setup/teardown network buffering [and 1 more messages]

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

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