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

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

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

> > > > On 17.02.15 at 19:32, <tamas.lengyel@zentific.com> wrote:
> > On Tue, Feb 17, 2015 at 3:31 PM, Jan Beulich <JBeulich@suse.com> wrote:
> > > > > > On 13.02.15 at 17:33, <tamas.lengyel@zentific.com> 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@lists.xen.org
http://lists.xen.org/xen-devel