From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38017) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFI3P-0000eZ-87 for qemu-devel@nongnu.org; Tue, 21 Jun 2016 05:33:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFI3L-0002VL-11 for qemu-devel@nongnu.org; Tue, 21 Jun 2016 05:33:06 -0400 Date: Tue, 21 Jun 2016 10:32:52 +0100 From: Stefan Hajnoczi Message-ID: <20160621093252.GF32560@stefanha-x1.localdomain> References: <1466016055-31351-1-git-send-email-clord@redhat.com> <20160617095451.GC6994@stefanha-x1.localdomain> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="+ts6NCQ4mrNQIV8p" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] Dynamic module loading for block drivers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Colin Lord Cc: kwolf@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com, den@openvz.org --+ts6NCQ4mrNQIV8p Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jun 20, 2016 at 11:32:38AM -0400, Colin Lord wrote: > On 06/17/2016 05:54 AM, Stefan Hajnoczi wrote: > > On Wed, Jun 15, 2016 at 02:40:53PM -0400, Colin Lord wrote: > >> 1) Denis Lunev suggested having block_module_load_one return the > >> loaded driver to reduce duplicated for loops in many of the functions > >> in block.c. I'd be happy to do this but wasn't completely sure how > >> error handling would happen in that case since currently the return > >> value is an integer error code. Would I switch to using the > >> error handling mechanisms provided in util/error.c? > >=20 > > Yes, change "int foo(...)" to "MyObject *foo(..., Error **errp)". The > > Error object allows functions to provide detailed, human-readable error > > messages so it can be a win. > >=20 > > If this change would cause a lot of changes you can stop the refactoring > > from snowballing using error_setg_errno() to bridge new Error functions > > with old int -errno functions: > >=20 > > MyObject *foo(..., Error **errp) > > { > > /* I don't want propagate Error to all called functions yet, it > > * would snowball. So just wrap up the errno: > > */ > > ret =3D legacy_function(...); > > if (ret < 0) { > > error_setg_errno(errp, -ret, "legacy_function failed"); > > return NULL; > > } > > } > >=20 >=20 > So I started implementing this part (having block_module_load_one return > the module/driver) last Friday and in the process I realized that it is > not as simple as it seemed to me at first. The duplicated for loops this > was supposed to fix aren't the nicest thing, but I don't think that > returning the block/module directly is any better. >=20 > The issue is that a module may contain multiple drivers, so > block_module_load_one would have to know exactly which driver to return, > which seems rather out of scope for that function. The function > registers multiple drivers when the module is loaded, so choosing just > one of them to return seems a little odd. >=20 > An alternative way to do this is to return the entire module rather than > just the driver, and let the caller figure out which driver it needs > from the module. However, that would require a loop of some sort anyway > to examine all the drivers in the module, so we're kind of back where we > started. But it is actually a little worse than where we started I think > because as far as I can tell, to actually access the drivers through the > module, you need to know the name of the symbol you want (which I > believe is the name of the BlockDriver structs). I don't see a good way > to know the exact name of the struct that would be robust, so at this > point it seems like it may be better to just leave the duplicated for > loops in place. I think the issue comes from the fact that you are considering something like load_block_module(const char *filename) as the API instead of request_block_driver(const char *driver_name). In the latter case it's possible to return a BlockDriver pointer. In the former it's not. The request_block_driver() approach requires a mapping from block driver names to modules. This can be achieved using a directory layout with symlinks (hmm...Windows portability?): /usr/lib/qemu/block/ +--- sheepdog.so +--- by-protocol/ +--- sheepdog+unix -> ../sheepdog.so request_block_driver() would look at /usr/lib/qemu/block/by-protocol/ to find the module file. Stefan --+ts6NCQ4mrNQIV8p Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJXaQnEAAoJEJykq7OBq3PIYMIIALoO2aRmzQJ6p+vZo2PM5fJA WUQ58zqWBAZzJjBoxWv6AbxA9JpEIg+W/5LBG4gBZKy91etseBopQCp9wlJg+AaW 1ZCfVC83+r+oSAbluJJn782vqlddOPraulGB2rgcIlqSGDWllueLLv4zn/lfhHhE SDtDUn87RIwRVSVnn6w+oYJvyijLIXG2Qiwo6N2A6AKiw1y1uemej0EBkvxlr0my xYDb6zSZApvi4TpiRVdQfVLJb5H2DWeY1XiipyGDnSoaQLZ/vtzfGNEdz05wJ1Am gkfLsnJI9NwKefEp+ar/irblGIm77qrtibofvH1bC6RuMkJfDqqgjYD05LS3MlY= =CaMe -----END PGP SIGNATURE----- --+ts6NCQ4mrNQIV8p--