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/<protocol> to find the module file.
Stefan
signature.asc
Description: PGP signature
