Re: [Xen-devel] [PATCH v6 4/9] ioreq-server: on-demand creation of ioreq server

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 08 May 2014 15:55
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org)
> Subject: Re: [PATCH v6 4/9] ioreq-server: on-demand creation of ioreq
> server
> 
> >>> On 08.05.14 at 15:23, <paul.durrant@xxxxxxxxxx> wrote:
> > +static int hvm_ioreq_server_map_pages(struct hvm_ioreq_server *s)
> > +{
> > +    struct domain *d = s->domain;
> > +    unsigned long pfn;
> > +    int rc;
> > +
> > +    pfn = d->arch.hvm_domain.params[HVM_PARAM_IOREQ_PFN];
> > +    rc = hvm_map_ioreq_page(s, 0, pfn);
> > +    if ( rc )
> > +        goto fail1;
> > +
> > +    pfn = d->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_PFN];
> > +    rc = hvm_map_ioreq_page(s, 1, pfn);
> > +    if ( rc )
> > +        goto fail2;
> > +
> > +    return 0;
> > +
> > +fail2:
> > +    hvm_unmap_ioreq_page(s, 0);
> > +
> > +fail1:
> > +    return rc;
> > +}
> 
> I thought we settled on permitting goto for _complex_ error handling, not
> trivial cases like this.
> 

I didn't think so. IMO a sequence of labels is still the best way to back out 
in all error handling.

> > +static void hvm_destroy_ioreq_server(struct domain *d)
> > +{
> > +    struct hvm_ioreq_server *s;
> > +
> > +    domain_pause(d);
> > +    spin_lock(&d->arch.hvm_domain.ioreq_server_lock);
> > +
> > +    s = d->arch.hvm_domain.ioreq_server;
> > +    if ( !s )
> > +        goto done;
> > +
> > +    d->arch.hvm_domain.ioreq_server = NULL;
> > +
> > +    hvm_ioreq_server_deinit(s);
> > +
> > + done:
> 
> Again.
> 
> > +    spin_unlock(&d->arch.hvm_domain.ioreq_server_lock);
> > +    domain_unpause(d);
> > +
> > +    if ( s )
> > +        xfree(s);
> 
> Pointless if().
> 

True. I'll get rid of it.

  Paul

> Jan


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