Re: [Xen-devel] [PATCH 18 of 27 v2] libxl: merge libxl__device_del into libxl__device_remove

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2011-10-17 at 15:34 +0100, Roger Pau Monnà wrote:

> > @@ -446,6 +395,67 @@ static int wait_for_dev_destroy(libxl__g
> >     return rc;
> >  }
> >
> > +/* Returns 0 on success, ERROR_* on fail */
> > +int libxl__device_remove(libxl__gc *gc, libxl__device *dev, int wait)
> > +{
> > +    libxl_ctx *ctx = libxl__gc_owner(gc);
> > +    xs_transaction_t t;
> > +    char *be_path = libxl__device_backend_path(gc, dev);
> > +    char *state_path = libxl__sprintf(gc, "%s/state", be_path);
> > +    char *state = libxl__xs_read(gc, XBT_NULL, state_path);
> > +    int rc = 0;
> > +
> > +    if (!state)
> > +        goto out;
> > +    if (atoi(state) != 4) {
> > +        libxl__device_destroy_tapdisk(gc, be_path);
> > +        xs_rm(ctx->xsh, XBT_NULL, be_path);
> 
> I think here we should return something different than 0 (possibly 1?)
> so the number of watches (n_watches) is not increased.

Yes, I think you are correct. We need to distinguish 3 cases: error,
success--event pending and success--no event pending. But this patch
only considers 2 of them.

Previously there was a model of returning the number of events which are
pending which I think makes sense (I understand what that was for
now ;-)), so the returns would become ERROR_* (all -ve), 0 (success--no
event pending) and 1 (success--event pending).

> > +        goto out;
> > +    }
> > +
> > +retry_transaction:
> > +    t = xs_transaction_start(ctx->xsh);
> > +    xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/online", be_path), "0", 
> > strlen("0"));
> > +    xs_write(ctx->xsh, t, state_path, "5", strlen("5"));
> > +    if (!xs_transaction_end(ctx->xsh, t, 0)) {
> > +        if (errno == EAGAIN)
> > +            goto retry_transaction;
> > +        else {
> > +            rc = ERROR_FAIL;
> > +            goto out;
> > +        }
> > +    }
> > +
> > +    xs_watch(ctx->xsh, state_path, be_path);
> > +    libxl__device_destroy_tapdisk(gc, be_path);
> > +
> > +    if (wait) {
> > +        struct timeval tv;
> > +        tv.tv_sec = LIBXL_DESTROY_TIMEOUT;
> > +        tv.tv_usec = 0;
> > +        (void)wait_for_dev_destroy(gc, &tv);
> > +        xs_rm(ctx->xsh, XBT_NULL, libxl__device_frontend_path(gc, dev));
> 
> Here we should check if the device is removed correctly or not, so
> that the number of watches is not increased:
> 
> if(wait_for_dev_destroy(gc, &tv) != 0) /* device destroyed */
>     rc = 1;

Agreed.

> > +    }
> > +
> > +    rc = 0;
> 
> This should also be removed, since rc is initialized to 0 already.

Right. Thanks for your review.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel