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

[Jeff Cody]

On Thu, Sep 08, 2016 at 10:34:24AM +0100, Daniel P. Berrange wrote:
> 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.
> 
> Yeah, and as you say, this approach is just plain wrong, and absolutely
> can never be merged as is. The external library must never use anything
> in QEMU that it wasn't explicitly given via a callback, otherwise that
> library is defacto part of QEMU.
>

Looking at the qnio test server, it does not link against the shim; it looks
like only QEMU links against the shim.

There are at least 3 qemu symbols referenced by the shim library (as of v5):
vxhs_dec_acb_segment_count, vxhs_inc_acb_segment_count, vxhs_set_acb_buffer.

So given that:

1. QEMU is the only user of the shim,
2. The shim calls QEMU functions directly,
3. The shim only exists to bridge QEMU and the QNIO library,
   (From the shim .c file:
   "Network IO library for VxHS QEMU block driver ")
4. The shim makes assumptions about QEMU (e.g. sector size alignment)

I strongly agree with everyone that the shim should just be part of QEMU,
and is the better choice over simply refactoring out QEMU calls from the
shim.


Jeff