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
|
Home