Re: [Xen-devel] [PATCH] vm_event: Add a new opcode to get VM_EVENT_INTERFACE_VERSION

[Petre Ovidiu PIRCALABU <ppircalabu@xxxxxxxxxxxxxxx>]

On Wed, 2019-02-13 at 08:27 -0700, Jan Beulich wrote:
> > > > On 13.02.19 at 14:25, <ppircalabu@xxxxxxxxxxxxxxx> wrote:
> > 
> > @@ -592,6 +592,19 @@ int vm_event_domctl(struct domain *d, struct
> > xen_domctl_vm_event_op *vec,
> >  {
> >      int rc;
> >  
> > +    if ( vec->op == XEN_VM_EVENT_GET_INTERFACE_VERSION )
> > +    {
> > +        vec->u.get_interface_version.value =
> > VM_EVENT_INTERFACE_VERSION;
> > +        return 0;
> > +    }
> > +
> > +    if ( unlikely(d == NULL) )
> > +    {
> > +        gdprintk(XENLOG_INFO,
> > +                 "Tried to do a memory event op on an invalid
> > domain.\n");
> > +        return -EINVAL;
> > +    }
> 
> To be compatible with previous behavior you want to return
> -ESRCH here. I'm also unconvinced of the need to add a log
> message here - there was none before in that case. But I'm
> not a maintainer of this code.
I will update the patch and post a new version.
> 
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -778,9 +778,10 @@ struct xen_domctl_gdbsx_domstatus {
> >   * to the hypervisor to pull responses (resume) from the given
> >   * ring.
> >   */
> > -#define XEN_VM_EVENT_ENABLE               0
> > -#define XEN_VM_EVENT_DISABLE              1
> > -#define XEN_VM_EVENT_RESUME               2
> > +#define XEN_VM_EVENT_ENABLE                 0
> > +#define XEN_VM_EVENT_DISABLE                1
> > +#define XEN_VM_EVENT_RESUME                 2
> > +#define XEN_VM_EVENT_GET_INTERFACE_VERSION  3
> 
> Perhaps just XEN_VM_EVENT_GET_VERSION?
I agree, it's simpler. I have used "INTERFACE" only to match the
VM_EVENT_INTERFACE_VERSION macro.

> 
> > @@ -843,7 +844,15 @@ struct xen_domctl_vm_event_op {
> >      uint32_t       op;           /* XEN_VM_EVENT_* */
> >      uint32_t       mode;         /* XEN_DOMCTL_VM_EVENT_OP_* */
> >  
> > -    uint32_t port;              /* OUT: event channel for ring */
> > +    union {
> > +        struct {
> > +            uint32_t port;       /* OUT: event channel for ring */
> > +        } enable;
> > +
> > +        struct {
> > +            uint32_t value;
> > +        } get_interface_version;
> 
> Why the wrapper structs? Having just a "port" and "version"
> field inside the union would be good enough, wouldn't it? But
> even if you want to stick to that, the new structure's name
> could be simply "version", thus also allowing re-use for a
> hypothetical "set-version" operation (in case multiple versions
> would want/need to be supported going forward).
I chose to wrap the "port" field in a structure because it's valid only
for the XEN_VM_EVENT_ENABLE operation (I have used the wrapping structs
in order to match the operation name). 
The "version" domctl should only return the vm_event version
(VM_EVENT_INTERFACE_VERSION is a macro and is defined at compile time).
At least in this case I think the backward compatibility issues should
be handled only at the client level (easier maintainance and
deployment).

Many thanks for your comments,
Petre


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