3:24 p.m.
On Fri, Sep 08, 2023 at 14:51:14 -0500, Jonathon Jongsma wrote:
On 7/24/23 8:05 AM, Peter Krempa wrote:
> As I've promised a long time ago I gave your patches some testing in
> regards of cooperation with blockjobs and snapshots.
>
> Since the new version of the patches was not yet posted on the list I'm
> replying here including my observations from testing patches from your
> gitlab branch:
>
> On Tue, Jun 06, 2023 at 16:15:30 -0500, Jonathon Jongsma wrote:
> > Requires recent qemu with support for the virtio-blk-vhost-vdpa device
> > and the ability to pass a /dev/fdset/N path for the vdpa path (8.1.0)
> >
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1900770
>
> Since this is a feature addition the 'Fixes' keyword doesn't make
sense.
> Use e.g. 'Resolves' instead.
>
> Additionally you're missing the DCO certification here.
>
> > ---
> > src/qemu/qemu_block.c | 20 ++++++++--
> > src/qemu/qemu_domain.c | 25 ++++++++++++
> > src/qemu/qemu_validate.c | 44 +++++++++++++++++++---
> > tests/qemuxml2argvdata/disk-vhostvdpa.args | 35 +++++++++++++++++
> > tests/qemuxml2argvdata/disk-vhostvdpa.xml | 21 +++++++++++
> > tests/qemuxml2argvtest.c | 2 +
> > 6 files changed, 139 insertions(+), 8 deletions(-)
> > create mode 100644 tests/qemuxml2argvdata/disk-vhostvdpa.args
> > create mode 100644 tests/qemuxml2argvdata/disk-vhostvdpa.xml
>
> [...]
>
> > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > index 2f6b32e394..119e52a7d7 100644
> > --- a/src/qemu/qemu_domain.c
> > +++ b/src/qemu/qemu_domain.c
> > @@ -11157,6 +11157,28 @@ qemuDomainPrepareStorageSourceFDs(virStorageSource
*src,
> > }
> > +static int
> > +qemuDomainPrepareStorageSourceVDPA(virStorageSource *src,
> > + qemuDomainObjPrivate *priv)
> > +{
> > + qemuDomainStorageSourcePrivate *srcpriv = NULL;
> > + virStorageType actualType = virStorageSourceGetActualType(src);
> > + int vdpafd = -1;
> > +
> > + if (actualType != VIR_STORAGE_TYPE_VHOST_VDPA)
> > + return 0;
> > +
> > + if ((vdpafd = qemuVDPAConnect(src->path)) < 0)
> > + return -1;
>
> This function call directly touches the host filesystem, which is not
> supposed to be in the *DomainPrepareStorageSource* functions but we
> rather have a completely separate machinery in
> qemuProcessPrepareHostStorage.
>
> Unfortunately that one doesn't yet need to handle individual backing
> chain members though.
>
> This ensures that the code doesn't get accidentally called from tests
> even without mocking the code as the tests reimplement the functions
> differently for testing purposes.
Somehow I missed this comment earlier. Unfortunately, it doesn't seem
straightforward to move this code. We can't simply move all of the logic to
qemuProcessPrepareHostStorage() because that function doesn't get called
during the tests.
That is the exact idea of it. This must not be called from tests because
you must not attempt to rely on host state during tests.
To make tests work, in testCompareXMLToArgvCreateArgs, we then setup a
fake FD to be passed to the command line generator. Similarly other
stuff is mocked there, mostly FD passing-related.
Additionally qemuProcessPrepareHostStorage() is also skipped inside the
domxmlToNative API as you also don't want to actually attempt opening
the VDPA socket in case when it won't be used. In that case mocking via
LD_PRELOAD is not possible.
In certain cases (if we care enough about the domxml->native API) we
even setup an alternative syntax (e.g. not using FD passing) so that the
users can adapt and use such commandline as inspiration to actually run
qemu.
I thought I could move only the opening of the fd to the
PrepareHostStorage() function and then keep the qemuFDPass construction in
this function, but that doesn't work: the PrepareHostStorage() function
actually gets called *after* this function. So the fd would not even be open
yet at the time this function gets called.
So... it seems that the options are either:
- leave everything in qemuDomainPrepareStorageSourceVDPA() (as is)
No. That would violate the idea of qemuProcessPrepareHostStorage().
- move the fd opening to PrepareHostStorage() and then move the rest
to a
different common function that is called after that, such as
qemuBuildDiskSourceCommandLine()
Inside PrepareHostStorage() you both open the FD and create the fdpass
structure and assign it to the private data, so there's no need for any
other location to do anything else.
- a third option (suggestions?)
It's worth noting that the vdpa *network* device essentially does everything
(opening the file, creating the qemuFDPass object, etc) in the
qemuBuildInterfaceCommandLine() function. This was done to match other
network devices that connect to open file descriptors (see
qemuBuildInterfaceConnect()). But based on your comments above, it sounds
like this may not be a the ideal situation even though the function is
mocked to not actually open any file descriptors from the host filesystem
under test.
Hmm, that is right, the function will be mocked, but regardless to not
make the code obscure, please make host-dependant storage backend setup
in the aforementioned function and whatever required to "mock" it
(including even calling qemuVDPAConnect if you wanto rely on the
LD_PRELOAD-ed version) in the tests.