From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olaf Hering Subject: Re: [PATCH v3 4/4] libxl: add support for vscsi Date: Fri, 6 Mar 2015 16:25:54 +0100 Message-ID: <20150306152554.GD21639@aepfle.de> References: <1425635156-2357-1-git-send-email-olaf@aepfle.de> <1425635156-2357-5-git-send-email-olaf@aepfle.de> <20150306143125.GX12103@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20150306143125.GX12103@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: Stefano Stabellini , Ian Jackson , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Fri, Mar 06, Wei Liu wrote: > I think you need to fix some overly long lines. I won't mention them > individually inline. Some are just copy&paste from other places. I will check what can be trimmed. > Regarding all the parsing stuffs, you haven't defined vscsispec so I > cannot review it. You might want to look at > docs/misc/xl-disk-configuration.txt. Its in pvscsi.txt, not complete yet. > I'm not suggesting you have to write something like that, but > considering all the compatibility issues you might have you might > actually end up writing up a document like that. I will check if its needed. > Then after that, do you consider writing a lexer for vscsispec? > We have one for diskspec, see libxlu_disk_l.l. That might result in a > shorter patch? The bulk is not the parser but using the result of the parsing. Or do you spot something that is better done with flex? > On Fri, Mar 06, 2015 at 10:45:56AM +0100, Olaf Hering wrote: > > Port pvscsi support from xend to libxl. See pvscsi.txt for details. > > Outstanding work is listed in the TODO section. > There is no TODO section in this patch. :-) See the line above, its in pvscsi.txt. > Better split this patch into two. One for libxl and one for xl. > > Or you can even split it into three, one for introducing libxl types, > one for implementing functionalities in libxl and one for xl. Would that actually improve things? In another thread it was suggested to have it all in a single patch to avoid jumping around between mails. > > +void libxl__device_vscsi_add(libxl__egc *egc, uint32_t domid, > > + libxl_device_vscsi *vscsi, > > + libxl__ao_device *aodev) > You need to update this domain's JSON configuration. Cf. > libxl__device_vtpm_add and friends. Also look at libxl_internal.h L2310. So I need to add my function to the comment near DEFINE_DEVICE_ADD()? Or do you mean something else? > > +{ > > + STATE_AO_GC(aodev->ao); > > + libxl_ctx *ctx = libxl__gc_owner(gc); > > + flexarray_t *front; > > + flexarray_t *back; > > + libxl__device *device; > > + char *be_path; > > + unsigned int be_dirs = 0, rc, i; > > + > > + if (vscsi->devid == -1) { > > + rc = ERROR_FAIL; > > + goto out; > > + } > > + > > + /* Prealloc key+value: 4 toplevel + 4 per device */ > > + i = 2 * (4 + (4 * vscsi->num_vscsi_devs)); > > + back = flexarray_make(gc, i, 1); > > + front = flexarray_make(gc, 2 * 2, 1); > > + > > + GCNEW(device); > > + rc = libxl__device_from_vscsi(gc, domid, vscsi, device); > > + if ( rc != 0 ) goto out; > > + > Coding style. No space after ( and before ). You can even just use > if (!rc) goto out; Copy&paste from some similar code from staging-4.4: libxl__device_vtpm_add > > + /* Check if backend device path is already present */ > > + be_path = libxl__device_backend_path(gc, device); > > + if (!libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs) || !be_dirs) { > > + /* backend does not exist, create a new one */ > > + flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid)); > > + flexarray_append_pair(back, "online", "1"); > > + flexarray_append_pair(back, "state", "1"); > > + flexarray_append_pair(back, "feature-host", GCSPRINTF("%d", !!vscsi->feature_host)); > > + > > + flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid)); > > + flexarray_append_pair(front, "state", "1"); > > + } > > + > > + for (i = 0; i < vscsi->num_vscsi_devs; i++) { > > + libxl_vscsi_dev *v = vscsi->vscsi_devs + i; > > + /* Trigger removal, otherwise create new device */ > Why do you want to trigger removal in "add" function? Because the vscsi is a single_host:many_devices, contrary to single_host:singe_device for other backends. > Please avoid using goto to retry transaction. Just copy&paste from other code, just grep for "retry_transaction". > > +void libxl_device_vscsi_append_dev(libxl_ctx *ctx, libxl_device_vscsi *hst, > > + libxl_vscsi_dev *dev) > > +{ > > Can this only be an internal function? I.e. use libxl__ namespace. Ok, will add the underscore. Olaf