[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v12 3/7] remus: introduce remus device
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. > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |