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/<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).
