Re: [Xen-devel] [PATCH v2 07/18] libxl: separate device add/rm complete callbacks

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Aug 27, 2014 at 02:41:55AM +0100, Ian Campbell wrote:
> On Wed, 2014-07-30 at 19:23 +0100, Wei Liu wrote:
> > This is in preparation for device hotplug / unplug configuration
> > synchronisation. No functional change introduced. This change is
> > necessary because rm is type specific, using a single callback cannot
> 
> Not "is" but "is going to become" I think?
> 

Yes, you're right.

> > meet our need.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> >  tools/libxl/libxl.c |   54 
> > +++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 42 insertions(+), 12 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 9ac2c3d..01dffa0 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -1798,22 +1798,26 @@ out:
> >  }
> >  
> >  
> > /******************************************************************************/
> > +#define DEVICE_AO_FAILED_MSG                                            \
> 
> I don't see why this couldn't be a static log_ao_failure message, but
> since it is used like a function it should be defined/called as
> DEVICE_AO_FAILED_MSG() so it looks normal. I think it should just be a
> function though.

OK, I will make it function.

> > @@ -3537,6 +3541,32 @@ out:
> >  }
> >  
> >  
> > /******************************************************************************/
> > +#define DEFINE_DEVICE_RM_AOCOMPLETE(type)                               \
> > +    static void device_##type##_rm_aocomplete(libxl__egc *egc,          \
> > +                                              libxl__ao_device *aodev)  \
> > +    {                                                                   \
> > +        STATE_AO_GC(aodev->ao);                                         \
> > +        if (aodev->rc) {                                                \
> > +            DEVICE_AO_FAILED_MSG;                                       \
> > +            goto out;                                                   \
> > +        }                                                               \
> > +                                                                        \
> > +    out:                                                                \
> 
> Perhaps you are going to add stuff between } and out: in a future patch,
> but if not then the goto+label are a bit pointless (I see now that you
> inherited that from the original code, so feel free to leave it if you
> prefer).
> 

I will leave it as it is, in case we have other things to add in between
goto and out label.

> > +        libxl__ao_complete(egc, ao, aodev->rc);                         \
> > +    }
> > +
> > +/*
> > + * device_vtpm_rm_aocomplete
> > + * device_nic_rm_aocomplete
> > + * device_disk_rm_aocomplete
> > + * device_vfb_rm_aocomplete
> > + * device_vkb_rm_aocomplete
> > + */
> > +DEFINE_DEVICE_RM_AOCOMPLETE(vtpm);
> > +DEFINE_DEVICE_RM_AOCOMPLETE(nic);
> > +DEFINE_DEVICE_RM_AOCOMPLETE(disk);
> > +DEFINE_DEVICE_RM_AOCOMPLETE(vfb);
> > +DEFINE_DEVICE_RM_AOCOMPLETE(vkb);
> 
> I was going to suggest merging this with DEFINE_DEVICE_REMOVE but that
> is used twice for each device so that won't work. But perhaps consider
> adding  these in the same blocks as the usage of DEFINE_DEVICE_REMOVE?
> 

They're already placed right before DEFINE_DEVICE_RM macro. You
requested this in your last review so I've done it already. ;-)

Wei.


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