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

[Thomas Huth]

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?

 Thanks,
  Thomas