From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60141) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFNpL-0004AB-9Y for qemu-devel@nongnu.org; Tue, 21 Jun 2016 11:43:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFNpJ-0002fD-56 for qemu-devel@nongnu.org; Tue, 21 Jun 2016 11:42:58 -0400 References: <1466016055-31351-1-git-send-email-clord@redhat.com> <20160617095451.GC6994@stefanha-x1.localdomain> <20160621093252.GF32560@stefanha-x1.localdomain> From: Colin Lord Message-ID: <42716482-40e8-64f3-8f57-f6bde71f4c20@redhat.com> Date: Tue, 21 Jun 2016 11:42:46 -0400 MIME-Version: 1.0 In-Reply-To: <20160621093252.GF32560@stefanha-x1.localdomain> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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: Stefan Hajnoczi Cc: kwolf@redhat.com, den@openvz.org, qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com On 06/21/2016 05:32 AM, Stefan Hajnoczi wrote: > 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? >>> >>> 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. >>> >>> 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: >>> >>> 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 = legacy_function(...); >>> if (ret < 0) { >>> error_setg_errno(errp, -ret, "legacy_function failed"); >>> return NULL; >>> } >>> } >>> >> >> 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. >> >> 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. >> >> 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 > The question is, even if I was using request_block_driver(const char *driver_name), I'm still not completely clear in your suggestion how I'm supposed to get the driver name in the first place. I don't see where I'd be getting that information from. I'd be happy to hear more about it, but it also made me think of another possible solution: right now the block_driver_modules array is being auto-generated by the python script. Why not just add a "name" field to the struct so that each array entry would actually know the name of the corresponding BlockDriver struct? Then request_block_driver would take in the array entry (which is a struct which currently has no name, which I will probably be fixing as Paolo asked) and it could load the file using the library name (as module_load_one does now). It could easily return the BlockDriver then using the name field. It seems to me that this would work, and would be a fairly minor change from how things are now (in particular I think that symlinks wouldn't be necessary with this).