[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 3/5] libxl: add support for vscsi
Just looking at the API functions on this pass. General comment though: Please go through and rewrap any lines longer than 75-80, I saw quite a few as I was scrolling. > @@ -3628,6 +3643,7 @@ static inline void libxl__update_config_vtpm(libxl__gc > *gc, > * devices have same identifier. */ > #define COMPARE_DEVID(a, b) ((a)->devid == (b)->devid) > #define COMPARE_DISK(a, b) (!strcmp((a)->vdev, (b)->vdev)) > +#define COMPARE_VSCSI(a, b) ((a)->v_hst == (b)->v_hst) Does this end up doing the correct thing given that vscsi is a bit different and the libxl_device_vscsi is actually a bus? Shouldn't something somewhere be taking care that vscsi->vscsi_devs is up to date, not just the list of the vscsi busses? > #define COMPARE_PCI(a, b) ((a)->func == (b)->func && \ > (a)->bus == (b)->bus && \ > (a)->dev == (b)->dev) > libxl_vscsi_dev = Struct("vscsi_dev", [ > + ("vscsi_dev_id", libxl_devid), > + ("remove", bool), What is this remove field? A libxl_vscsi_dev describes a device, not the actions which can be performed on the device. Those are in the names of the functions. > + ("pdev", libxl_vscsi_pdev), > + ("vdev", libxl_vscsi_hctl), > + ]) > + > +libxl_device_vscsi = Struct("device_vscsi", [ > + ("backend_domid", libxl_domid), > + ("devid", libxl_devid), > + ("v_hst", uint32), > + ("vscsi_devs", Array(libxl_vscsi_dev, "num_vscsi_devs")), > + ("next_vscsi_dev_id", libxl_devid), This one also seems odd. If you need to store state internally within libxl then you will have to arrange to store it somewhere else, the public API structs are not appropriate. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |