WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Roger Pau Monné <roger.pau@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH 18 of 27 v2] libxl: merge libxl__device_del into libxl__device_remove
From: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
Date: Mon, 17 Oct 2011 15:43:59 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Mon, 17 Oct 2011 07:45:27 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <CAPLaKK5wMHzRwqS207RG-2+FHox2zBBnf7+ROdRNxx0AoCLJsg@xxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <patchbomb.1318499605@xxxxxxxxxxxxxxxxxxxxx> <95b2f3977d439bba3a01.1318499623@xxxxxxxxxxxxxxxxxxxxx> <CAPLaKK5wMHzRwqS207RG-2+FHox2zBBnf7+ROdRNxx0AoCLJsg@xxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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

<Prev in Thread] Current Thread [Next in Thread>