Re: [Xen-devel] [PATCH 2/4] tools/libxc: Define VM_EVENT type

[Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>]

On Vi, 2018-09-14 at 03:14 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 13.09.18 at 17:01, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -757,10 +757,17 @@ struct xen_domctl_gdbsx_domstatus {
> >  
> >  /*
> >   * There are currently three rings available for VM events:
> > - * sharing, monitor and paging. This hypercall allows one to
> > - * control these rings (enable/disable), as well as to signal
> > - * to the hypervisor to pull responses (resume) from the given
> > - * ring.
> > + * sharing, monitor and paging
> > + */
> > +
> > +#define XEN_VM_EVENT_TYPE_PAGING 1
> > +#define XEN_VM_EVENT_TYPE_MONITOR 2
> > +#define XEN_VM_EVENT_TYPE_SHARING 3
> > +
> > +/*
> > + * This hypercall allows one to control the vm_event rings
> > (enable/disable),
> > + * as well as to signal to the hypervisor to pull responses
> > (resume) from
> > + * the given ring.
> >   */
> What's the reason to modify the comment, the more with a style
> violation (malformed single line comment) as the result?
> 
I have split the comment in 2 parts. The first ("There are ... sharing,
monitor and paging ") describes the 3 XEN_VM_EVENT_TYPE_* rings, and
the second describes the vm_event hypercalls ( XEN_VM_EVENT_ENABLE ....
). Other than the missing period at the end of "paging" (which I will
fix in the next iteration) I cannot identify other coding style
violations.

> > 
> > @@ -780,7 +787,7 @@ struct xen_domctl_gdbsx_domstatus {
> >   * EXDEV  - guest has PoD enabled
> >   * EBUSY  - guest has or had paging enabled, ring buffer still
> > active
> >   */
> > -#define XEN_DOMCTL_VM_EVENT_OP_PAGING            1
> > +#define XEN_DOMCTL_VM_EVENT_OP_PAGING XEN_VM_EVENT_TYPE_PAGING
> >  
> >  /*
> >   * Monitor helper.
> > @@ -804,7 +811,7 @@ struct xen_domctl_gdbsx_domstatus {
> >   * EBUSY  - guest has or had access enabled, ring buffer still
> > active
> >   *
> >   */
> > -#define XEN_DOMCTL_VM_EVENT_OP_MONITOR           2
> > +#define XEN_DOMCTL_VM_EVENT_OP_MONITOR XEN_VM_EVENT_TYPE_MONITOR
> >  
> >  /*
> >   * Sharing ENOMEM helper.
> > @@ -819,7 +826,7 @@ struct xen_domctl_gdbsx_domstatus {
> >   * Note that shring can be turned on (as per the domctl below)
> >   * *without* this ring being setup.
> >   */
> > -#define XEN_DOMCTL_VM_EVENT_OP_SHARING           3
> > +#define XEN_DOMCTL_VM_EVENT_OP_SHARING XEN_VM_EVENT_TYPE_SHARING
> And what's the reason to retain these (now redundant)
> XEN_DOMCTL_VM_EVENT_OP_* definitions? Either they're independent
> (in which case they shouldn't resolve to XEN_VM_EVENT_TYPE_*) or
> they're true aliases (tolerating arbitrary future changes to
> XEN_VM_EVENT_TYPE_* without further adjustment here), and then
> unnecessary to retain.
> 
> Jan
> 
> 
I kept them despite the redundancy because the domctl.h header is
public and I didn't wanted to break any existent (external) clients of
this interface. 
However, if you think removing them altogether it's best, I will
replace them with the XEN_VM_EVENT_TYPE_* and increment
XEN_DOMCTL_INTERFACE_VERSION.

//Petre

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel