Yang Hongyang writes ("[PATCH v12 3/7] remus: introduce remus device"):
introduce remus device, an abstract layer of remus devices(nic, disk,
etc).It provides the following APIs for libxl:
Thanks for this. Things are much clearer for me now. I have some
more detailed comments.
+struct libxl__remus_device_ops {
+ /*
+ * init() and destroy() APIs are produced by a device type and
+ * consumed by the main remus code, a device type must implement
+ * these two APIs.
+ */
+ /* init device ops private data, etc. must implement */
+ int (*init)(libxl__remus_device_ops *self,
+ libxl__remus_state *rs);
+ /* free device ops private data, etc. must implement */
+ void (*destroy)(libxl__remus_device_ops *self);
+ /*
+ * This is device ops's private data, for different device types,
+ * the data structs are different
+ */
+ void *data;
I see that in 4/7 you use this to store global state; it's not const.
libxl is not supposed to to have any global state that's not hung off
the libxl_ctx (or some similar) structure. So I think things like the
NIC data need to be in libxl_ctx.
You have two reasonable options, I think: either, include the
variables you need directly in the libxl_ctx. Or, make libxl_ctx
contain a pointer to a struct which is declared in libxl_internal.h
but only defined in (say) libxl_netbuffer.c. The latter is probably
better because it more easily allows different
platorms'/implementations' versions of the same device kind to have
different variables.
It is IMO fine for the different devices to each have their own
member in libxl_ctx.
All the libxl__remus_device_ops structs need to be const.
Taking as an example:
+ if (rds->num_devices == rds->num_setuped)
Can you please write "num_set_up" ? I'm afraid that "setuped" isn't a
word in English and it reads very oddly. Thanks.
+ /*
+ * This is for matching, we must go through all device ops until we
+ * find a matched op for the device. The ops_index record which ops
+ * we are matching.
+ */
+ int ops_index;
+ libxl__remus_device_ops *ops;
+ libxl__remus_device_callback *callback;
+ libxl__remus_device_state *rds;
I think this is confusing. Are all of these private for the abstract
layer ? I think the remus concrete device ought to be allowed to use
libxl__remus_device_ops (which needs to be const).
It is more important to say who owns a struct field (who is
allowed to read it; who is responsible for setting it, etc.) than to
say precisely what it is for.
If you're going to have both kinds of comments in the same struct
(that is, both sections which have particular ownership and individual
usage comments) it would be helpful to make ownership section comments
more obvious.
Perhaps something like:
struct libxl__remus_device {
/*----- shared between abstract and concrete layers -----*/
/* set by remus device abstruct layer */
int devid;
/* libxl__device_* which this remus device related to */
const void *backend_dev;
libxl__remus_device_kind kind;
/*----- private for abstract layer only -----*/
/*
* This is for matching, we must go through all device ops until we
* find a matched op for the device. The ops_index record which ops
* we are matching.
*/
int ops_index;
libxl__remus_device_ops *ops;
libxl__remus_device_callback *callback;
libxl__remus_device_state *rds;
/*----- private for concrete (device-specific) layer -----*/
/* *kind* of device's private data */
void *data;
/* for calling scripts, eg. setup|teardown|match scripts */
libxl__async_exec_state aes;
/*
* for async func calls, in the implenmentation of device ops, we
* may use fork to do async ops. this is owned by device-specific
* ops methods
*/
libxl__ev_child child;
};
Writing it like that shows another anomaly: why do we have fields in
the libxl__remus_device struct that are private for the device
specific layer ?
After all the device-specific layer has "data", which could contain
anything it likes.
Why is the libxl__remus_device_state a separate structure ?
Can it be combined with libxl__remus_device_state ?
+static libxl__remus_device_ops *dev_ops[] = {
+};
As I say, this needs to be const.
+static void device_common_cb(libxl__egc *egc,
+ libxl__remus_device *dev,
+ int rc)
When we write this kind of callback threaded code, we write it in
chronological order. That is:
- banner comment at the top
- necessary forward declarations (and data types)
- entrypoint
- callbacks, in order in which they are called
- banner comment for next section
See for example the utilities in libxl_aoutils.c.
Or for a bigger example see libxl_bootloader.c, in particular the
section after the comment "main flow of control" near line 304.
+static __attribute__((unused)) void libxl__remus_device_init(libxl__egc *egc,
+ libxl__remus_device_state *rds,
+ libxl__remus_device_kind kind,
+ void *libxl_dev)
...
+ switch (kind) {
+ case LIBXL__REMUS_DEVICE_NIC:
+ nic = libxl_dev;
+ dev->devid = nic->devid;
+ break;
+ case LIBXL__REMUS_DEVICE_DISK:
+ disk = libxl_dev;
+ /* there are no dev id for disk devices */
+ dev->devid = -1;
+ break;
Wouldn't it be better to move this into the concrete (device type
specific) code ? After all you're switching on the device type
already, and the device type specific code has all the same
information ?
Or am I missing something ?
+void libxl__remus_device_teardown(libxl__egc *egc, libxl__remus_state *rs)
+{
+ int i;
+ libxl__remus_device *dev;
+ libxl__remus_device_ops *ops;
+
+ STATE_AO_GC(rs->ao);
+
+ /* Convenience aliases */
+ libxl__remus_device_state *const rds = &rs->dev_state;
+
+ if (rds->num_setuped == 0) {
+ /* clean device ops */
+ for (i = 0; i < ARRAY_SIZE(dev_ops); i++) {
+ ops = dev_ops[i];
+ ops->destroy(ops);
+ }
+ goto out;
+ }
I was a bit confused by the nonlinear flow of control in this file,
but it is definitely clear to me that you have two copies of this
loop.
Why don't you just make the callback to your own callback function
yourself (under appropriate conditions) ?
