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

[Colin Lord]

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.