Re: [PATCH v8 4/4] new qTest case to test the vhost-user-blk-server

[Coiby Xu]

On Fri, Jun 05, 2020 at 11:25:26AM +0200, Thomas Huth wrote:
> On 05/06/2020 08.22, Coiby Xu wrote:
> > On Fri, Jun 05, 2020 at 07:01:33AM +0200, Thomas Huth wrote:
> > > > diff --git a/tests/qtest/libqos/vhost-user-blk.h
> > > > b/tests/qtest/libqos/vhost-user-blk.h
> > > > new file mode 100644
> > > > index 0000000000..ef4ef09cca
> > > > --- /dev/null
> > > > +++ b/tests/qtest/libqos/vhost-user-blk.h
> > > > @@ -0,0 +1,44 @@
> > > > +/*
> > > > + * libqos driver framework
> > > > + *
> > > > + * Copyright (c) 2018 Emanuele Giuseppe Esposito
> > > > <e.emanuelegiuseppe@gmail.com>
> > > > + *
> > > > + * This library is free software; you can redistribute it and/or
> > > > + * modify it under the terms of the GNU Lesser General Public
> > > > + * License version 2 as published by the Free Software Foundation.
> > >
> > > ... but you've missed the header here :-(
> >
> > Thank you for reminding me of this issue!
> >
> > > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > > > index 49075b55a1..a7b7c96206 100644
> > > > --- a/tests/qtest/libqtest.c
> > > > +++ b/tests/qtest/libqtest.c
> > > > @@ -31,40 +31,9 @@
> > > >  #include "qapi/qmp/qlist.h"
> > > >  #include "qapi/qmp/qstring.h"
> > > >
> > > > -#define MAX_IRQ 256
> > > >  #define SOCKET_TIMEOUT 50
> > > >  #define SOCKET_MAX_FDS 16
> > > >
> > > > -
> > > > -typedef void (*QTestSendFn)(QTestState *s, const char *buf);
> > > > -typedef void (*ExternalSendFn)(void *s, const char *buf);
> > > > -typedef GString* (*QTestRecvFn)(QTestState *);
> > > > -
> > > > -typedef struct QTestClientTransportOps {
> > > > -    QTestSendFn     send;      /* for sending qtest commands */
> > > > -
> > > > -    /*
> > > > -     * use external_send to send qtest command strings through
> > > > functions which
> > > > -     * do not accept a QTestState as the first parameter.
> > > > -     */
> > > > -    ExternalSendFn  external_send;
> > > > -
> > > > -    QTestRecvFn     recv_line; /* for receiving qtest command
> > > > responses */
> > > > -} QTestTransportOps;
> > > > -
> > > > -struct QTestState
> > > > -{
> > > > -    int fd;
> > > > -    int qmp_fd;
> > > > -    pid_t qemu_pid;  /* our child QEMU process */
> > > > -    int wstatus;
> > > > -    int expected_status;
> > > > -    bool big_endian;
> > > > -    bool irq_level[MAX_IRQ];
> > > > -    GString *rx;
> > > > -    QTestTransportOps ops;
> > > > -};
> > >
> > > Why do you have to move struct QTestState and friends to the header
> > > instead? I'd prefer if we could keep it here if possible?
> >
> > tests/qtest/vhost-user-blk-test.c needs to talk to qemu-storage-daemon's
> > QMP. Thus I g_new0 a QTestState struct to make use of related functions
> > like qtest_qmp and this requires the QTestState struct definition.
>
> Hm, ok, could that maybe be solved by introducing a wrapper function to
> libqtest.c instead? Something like qtest_create_state_with_qmp_fd() or so?
> Moving a define with a generic name like MAX_IRQ to a header really does
> not sound like a good idea to me, so if that idea with the wrapper
> function does not work out, could you please at least rename MAX_IRQ to
> QTEST_MAX_IRQ or something similar?
I didn't realize the QTestState struct is supposed to be hidden from the user 
and
not directly accessible. To typedef a struct in a header file and define
the struct in the c file is a new trick for me:)

This idea of creating a wrapper function qtest_create_state_with_qmp_fd
works as expected. Thank you!