Re: [PATCH v12 3/7] remus: introduce remus device

[Wen Congyang <wency@cn.fujitsu.com>]

At 06/30/2014 01:02 PM, Hongyang Yang Wrote:
> Hi Ian,
>   Thanks for the review!
> 
> On 06/28/2014 01:38 AM, Ian Jackson wrote:
>> 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.
> 
> I will take option 2.
> 
>>
>> 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.
> 
> Sorry for the bad English...will fix that.
> 
>>
>>
>>> +    /*
>>> +     * 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 ?
> 
> I think you mean combined with libxl__remus_device here, yes, it can be
> combined because it is a member of libxl__remus_device, I separate it
> because I want to make the structure more clear, but seems it is
> confusing here, I will combine it in the next version.

Hmm, I think this abstract layer can be reused by colo, and the ops can
not be reused.

So I think we should not combine it with libxl__remus_state.

Thanks
Wen Congyang

> 
>>
>>
>>> +static libxl__remus_device_ops *dev_ops[] = {
>>> +};
>>
>> As I say, this needs to be const.
> 
> Sure, thanks.
>