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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 04 March 2014 13:03
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v2 4/6] ioreq-server: on-demand creation of
> ioreq server
> 
> >>> On 04.03.14 at 12:40, Paul Durrant <paul.durrant@xxxxxxxxxx> wrote:
> > +static void hvm_init_ioreq_page(struct hvm_ioreq_server *s, int buf)
> >  {
> > +    struct hvm_ioreq_page *iorp;
> > +
> > +    iorp = ( buf ) ? &s->buf_ioreq : &s->ioreq;
> 
> Pointless parentheses and spaces (also further down).
> 

I general prefer to parenthesize the conditional as a nod to this essentially 
being an if-then-else. Is that outlawed?

> Also "buf" appears to be boolean, so please reflect this by using
> bool_t.
> 
> > @@ -557,38 +557,6 @@ static int handle_pvh_io(
> >      return X86EMUL_OKAY;
> >  }
> >
> > -static int hvm_init_ioreq_server(struct domain *d)
> > -{
> > -    struct hvm_ioreq_server *s;
> > -    int i;
> > -
> > -    s = xzalloc(struct hvm_ioreq_server);
> > -    if ( !s )
> > -        return -ENOMEM;
> > -
> > -    s->domain = d;
> > -
> > -    for ( i = 0; i < MAX_HVM_VCPUS; i++ )
> > -        s->ioreq_evtchn[i] = -1;
> > -    s->buf_ioreq_evtchn = -1;
> > -
> > -    hvm_init_ioreq_page(d, &s->ioreq);
> > -    hvm_init_ioreq_page(d, &s->buf_ioreq);
> > -
> > -    d->arch.hvm_domain.ioreq_server = s;
> > -    return 0;
> > -}
> > -
> > -static void hvm_deinit_ioreq_server(struct domain *d)
> > -{
> > -    struct hvm_ioreq_server *s = d->arch.hvm_domain.ioreq_server;
> > -
> > -    hvm_destroy_ioreq_page(d, &s->ioreq);
> > -    hvm_destroy_ioreq_page(d, &s->buf_ioreq);
> > -
> > -    xfree(s);
> > -}
> 
> Odd - you completely remove code here that an earlier patch of
> this series added, and you put it back in altered form further down.
> The original patch in this case would better add it in the final place
> right away, so that the diff actually tells the reader what changes.
> 

Well, it is slightly different code. I wanted a sequence where we moved to an 
ioreq server struct that was just an abstraction within the domain struct (and 
had the same lifecycle) to one that was created on-demand. This is why the code 
moves and changes name. It seems like a reasonable way to sequence things to 
me. I could, of course, do all this in a monolithic patch but it would probably 
be harder to review.

> > +static int hvm_create_ioreq_server(struct domain *d, domid_t domid)
> > +{
> > +    struct hvm_ioreq_server *s;
> > +    unsigned long pfn;
> > +    struct vcpu *v;
> > +    int i, rc;
> > +
> > +    rc = -EEXIST;
> > +    if ( d->arch.hvm_domain.ioreq_server != NULL )
> > +        goto fail_exist;
> 
> Please don't use goto-s without actual need.
> 
> > +
> > +    gdprintk(XENLOG_INFO, "%s: %d\n", __func__, d->domain_id);
> 
> That's a debugging left-over I assume?
> 

Yes.

> > +fail_add_vcpu:
> > +    for_each_vcpu ( d, v )
> > +        hvm_ioreq_server_remove_vcpu(s, v);
> > +    domain_unpause(d);
> > +    hvm_destroy_ioreq_page(s, 1);
> > +fail_set_buf_ioreq:
> > +    hvm_destroy_ioreq_page(s, 0);
> > +fail_set_ioreq:
> > +    xfree(s);
> > +fail_alloc:
> > +fail_exist:
> 
> Labels should be indented by one space.
> 

I couldn't find anything in CODING_STYLE concerning labels, but I'll follow 
suit.

> > +static void hvm_destroy_ioreq_server(struct domain *d)
> > +{
> > +    struct hvm_ioreq_server *s;
> > +    struct vcpu *v;
> > +
> > +    gdprintk(XENLOG_INFO, "%s: %d\n", __func__, d->domain_id);
> 
> Again a leftover?
> 

Yep - should be XENLOG_DEBUG.

> > @@ -697,27 +790,6 @@ done:
> >      return rc;
> >  }
> >
> > -static int hvm_set_ioreq_server_pfn(struct hvm_ioreq_server *s,
> unsigned long pfn)
> > -{
> > -    struct domain *d = s->domain;
> > -    int rc;
> > -
> > -    rc = hvm_set_ioreq_page(d, &s->ioreq, pfn);
> > -    if ( rc < 0 )
> > -        return rc;
> > -
> > -    hvm_update_ioreq_server_evtchn(s);
> > -
> > -    return 0;
> > -}
> > -
> > -static int hvm_set_ioreq_server_buf_pfn(struct hvm_ioreq_server *s,
> unsigned long pfn)
> > -{
> > -    struct domain *d = s->domain;
> > -
> > -    return hvm_set_ioreq_page(d, &s->buf_ioreq, pfn);
> > -}
> 
> Again code an earlier patch added getting moved around?
> 

For similar reasons.

  Paul

> Jan


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