[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH 0/2] Dynamic module loading for blo

From: Stefan Hajnoczi
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH 0/2] Dynamic module loading for block drivers
Date: Tue, 21 Jun 2016 10:32:52 +0100
User-agent: Mutt/1.6.1 (2016-04-27)

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?):

    +--- sheepdog.so
    +--- by-protocol/
      +--- sheepdog+unix -> ../sheepdog.so

request_block_driver() would look at
/usr/lib/qemu/block/by-protocol/<protocol> to find the module file.


Attachment: signature.asc
Description: PGP signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]