Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

[Paolo Bonzini]

On 08/09/2016 12:18, Kevin Wolf wrote:
> Am 08.09.2016 um 11:43 hat Daniel P. Berrange geschrieben:
>> On Thu, Sep 08, 2016 at 11:29:41AM +0200, Kevin Wolf wrote:
>>> Am 08.09.2016 um 10:49 hat Daniel P. Berrange geschrieben:
>>>> On Thu, Sep 08, 2016 at 10:43:19AM +0200, Kevin Wolf wrote:
>>>>> Am 08.09.2016 um 00:32 hat ashish mittal geschrieben:
>>>>>>>> +
>>>>>>>> +void vxhs_set_acb_buffer(void *ptr, void *buffer)
>>>>>>>> +{
>>>>>>>> +    VXHSAIOCB *acb = ptr;
>>>>>>>> +
>>>>>>>> +    acb->buffer = buffer;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>
>>>>>>> Unused function?
>>>>>>
>>>>>> This is called from within libqnio.
>>>>>
>>>>> Wait, you mean the library references a symbol in the qemu binary? This
>>>>> sounds completely wrong to me. I wouldn't even do something like this if
>>>>> it were an internal qemu library because I think libraries should be
>>>>> self-contained, but it's a much larger problem in something that is
>>>>> supposed to be an independent library.
>>>>
>>>> I'd be surprised if that actually worked. If an external library wants
>>>> to refrence symbols in the QEMU binary, then QEMU would ned to be using
>>>> the -export-dynamic / -rdynamic linker flags to export all its symbols,
>>>> and AFAIK, we're not doing that.
>>>
>>> Hm, but if the function is really used by the library, how else would it
>>> be when its name isn't mentioned anywhere in the patch except in its
>>> declaration? And it appears to be called there directly:
>>>
>>> https://github.com/MittalAshish/libqnio/search?q=vxhs_set_acb_buffer
>>>
>>> Anyway, something is fishy here.
>>
>> Oh, notice that .c file is actually part of the shim library, not the
>> main libqnio.so that QEMU would link against.
>>
>> So, it really is unused and should be deleted from this block/vxhs.c
>> file.
> 
> I haven't fully understood yet what's the deal with this shim library,
> but this patch links against both.

Indeed, I suspect that the "shim library" should really be part of QEMU.
 It's just 700 lines of code, and some parts (e.g.
qnio_ck_initialize_lock seem unused even).  The main thing to do is
convert it from cJSON to QEMU's internal libraries for the same thing.

Paolo