Re: [Xen-devel] [PATCH v5 01/10] libxl: change libxl__ao_device_remove to libxl__ao_device

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v5 01/10] libxl: change libxl__ao_device_remove 
to libxl__ao_device"):
> Introduce a new structure to track state of device backends, that will
> be used in following patches on this series.
...
> +_hidden void libxl__prepare_ao_device(libxl__ao_device *aodev, libxl__ao *ao,
> +                                      libxl__ao_device **base);

The doc comment doesn't seem to explain base.  What is it used for ?

I see only this:

> +void libxl__prepare_ao_device(libxl__ao_device *aodev, libxl__ao *ao,
> +                              libxl__ao_device **base)
...
> +    aodev->base = base;

but:

> +struct libxl__ao_device {
...
> +    /* private for implementation */
...
> +    void *base;
> +};

Why is it void* here when it's only ever set to a libxl__ao_device** ?


> +void libxl__initiate_device_remove(libxl__egc *egc,
> +                                   libxl__ao_device *aodev)
...
> -    libxl__ev_devstate_init(&aorm->ds);
> +    libxl__ev_devstate_init(&aodev->ds);
> 
> -    rc = libxl__ev_devstate_wait(gc, &aorm->ds, device_remove_callback,
> +    rc = libxl__ev_devstate_wait(gc, &aodev->ds, device_backend_callback,
>                                   state_path, XenbusStateClosed,
>                                   LIBXL_DESTROY_TIMEOUT * 1000);

libxl__ev_devstate_init is not needed before _wait.  (Admittedly
that's there in the code before you touched it.)  In general foo_init
is not needed before foo_do_the_thing_and_call_me_back.

Given the use of `backend' in function names to refer to what is
manipulated with aodev->ds, perhaps aodev->ds should be called
aodev->backend_ds ?

I was briefly confused about whether `device_backend_cleanup' was a
general cleanup function for a libxl__ao_device (which it isn't).  And
there is of course a frontend state too, which we don't explicitly
deal with here.

How do the frontend and backend directories get removed from
xenstore ?  (I have a feeling I have asked this before but I don't
remember the answer.  Perhaps it could be a comment in the code in
some appropriate place?)


> +/* This functions sets the necessary libxl__ao_device struct values to use
> + * safely inside functions. It marks the operation as "active"

This `active' business is not used in this patch.  The variable and
its initialisation and commentary should be introduced in the same
patch as its use.

So either the ancillary functions which use `active' need to be
brought forward and documented here, or the introduction of `active'
should be postponed.

> +struct libxl__ao_device {
> +    /* filled in by user */
> +    libxl__ao *ao;
> +    /* action being performed */
> +    libxl__device_action action;
> +    libxl__device *dev;
> +    int force;
> +    libxl__device_callback *callback;
> +    /* private for implementation */
> +    int active;
...

This is a bit confusing.  I would remove the comment `/* action being
performed */' because it seems to refer to all the following variables
(and to disapply `/* filled in by user */' which does in fact apply).
And the only thing it seems to be actually trying to document is that
the variable `action' is the action, which is a bit pointless.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel