Re: [Xen-devel] [RFC XEN PATCH for-4.13 2/4] libxl: Introduce libxl__ev_qmplock

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Thanks.  I approve of the general approach, and the code reuse, but I
have some qualms about the resulting layering structure and the
boilerplate wrappers.  I have some suggestions for how this might look
better.

Anthony PERARD writes ("[RFC XEN PATCH for-4.13 2/4] libxl: Introduce 
libxl__ev_qmplock"):
> This lock will be used to prevent concurrent access the QEMU's QMP
> socket. It is based on libxl__ev_devlock implementation and have the
> same properties.
...
> +void libxl__ev_qmplock_init(libxl__ev_qmplock *lock)
> +{
> +    libxl__ev_devlock_init(lock);
> +}
> +
> +void libxl__ev_qmplock_lock(libxl__egc *egc, libxl__ev_qmplock *lock)
> +{
> +    ev_lock_lock(egc, lock, "qmp-socket-lock");
> +}

This produces a lot of rather pointless functions.  Also the layering
is anomalous: one of these locks is primary and most of the calls for
the other are implemented in terms of the other.

One possible alternative approach would be as follows:

1. Rename devlock to slowlock everywhere.  Expect everyone including
   qmp to call libxl__ev_slowlock_*.

2. Perhaps, put const char *userdata_userid in the lock structure.
   Have it set by libxl__ev_slowlock_init rather than by _lock.  (This
   centralises things a bit and may reduce duplication or improve
   error messages or something.)

3. Perhaps wrap up libxl__ev_slowlock_init with two functions
   [libxl__ev_]devlock_init and libxl__ev_qmplock_init, and rename
   libxl__ev_slowlock_init to libxl__ev_slowlock_init_internal.

This avoids having to provide trivial wrappers for all the functions.
if you do all of this including (3) then the API is slightly anomalous
in that there are several distinct init functions but only one set of
operation functions but this seems OK to me.

What do you think ?

Thanks,
Ian.

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