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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.09.18 at 13:11, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> 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.

I realize you did this, but you still don't really clarify why.

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

Well, I've already told you: The first of the two resulting comments
is now supposed to be all on a single line.

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

Yes, that's what I think should be happening, unless there were other
reasons.

Jan



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