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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.03.14 at 14:30, Paul Durrant <Paul.Durrant@xxxxxxxxxx> wrote:
>>  -----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?

It's contrary to how all other code does it - optionally use
parentheses when the condition is an more involved expression,
but don't use them when it's a simple identifier or a simple
(generally taken to mean unary) expression.

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

Indeed it would be. What I'm asking for is that you put the code
in the right place from the beginning, so that the patch then shows
the actual change you make to it.

>> > +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.

The main reason for this is to not have the labels show up as -p
diff/patch context - we want the function name here.

>> > +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.

Not even that imo.

Jan


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