Re: [Xen-devel] [PATCH V5 12/12] xen/vm_event: Add RESUME option to vm_event_op domctl

[Tamas K Lengyel <tamas.k.lengyel@xxxxxxxxx>]

On Wed Feb 18 2015 10:31:06 AM CET, Jan Beulich <JBeulich@xxxxxxxx> wrote:

> > > > On 17.02.15 at 19:32, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> > On Tue, Feb 17, 2015 at 3:31 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> > > > > > On 13.02.15 at 17:33, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> > > > @@ -611,13 +611,22 @@ int vm_event_domctl(struct domain *d, 
> > xen_domctl_vm_event_op_t *vec,
> > > > }
> > > > break;
> > > > 
> > > > -Â Â Â Â Â Â Â  case XEN_VM_EVENT_PAGING_DISABLE:
> > > > +Â Â Â Â Â Â Â  case XEN_VM_EVENT_DISABLE:
> > > > {
> > > > if ( ved->ring_page )
> > > > rc = vm_event_disable(d, ved);
> > > > }
> > > > break;
> > > > 
> > > > +Â Â Â Â Â Â Â  case XEN_VM_EVENT_RESUME:
> > > > +Â Â Â Â Â Â Â  {
> > > > +Â Â Â Â Â Â Â Â Â Â Â  if ( ved->ring_page )
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  vm_event_resume(d, ved);
> > > > +Â Â Â Â Â Â Â Â Â Â Â  else
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  rc = -ENODEV;
> > > > +Â Â Â Â Â Â Â  }
> > > > +Â Â Â Â Â Â Â  break;
> > > 
> > > Stray braces again.
> > 
> > Ack.
> > 
> > > 
> > > I also find it confusing that the same set of changes repeats three
> > > times here - is that an indication of a problem with an earlier
> > > patch?
> > 
> > No it's not. There are three rings vm_event can use, thus three rings
> > that can be resumed.
> 
> But if the code ends up being almost identical, this loudly calls for
> consolidation into e.g. a helper function.
> 
> Jan

That could be done but considering we are talking about only couple lines of 
code, I'm not sure that would improve readability by much.

I think the question I raised earlier, whether we need the resume option to the 
domctl to begin with is what needs discussion. IMHO the event channel method is 
enough so maybe I'll just turn this patch into deprecating the resume options 
in the memops.

Thanks,
Tamas

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