Re: [PATCH v3 13/22] libqtest: make qtest_bufwrite send "atomic"

["Oleinik, Alexander" <alxndr@bu.edu>]

[-- Attachment #1: Type: text/plain, Size: 3928 bytes --]

On Thu, 2019-09-19 at 14:56 -0400, John Snow wrote:
>
> On 9/19/19 6:37 AM, Stefan Hajnoczi wrote:
> > On Wed, Sep 18, 2019 at 11:19:40PM +0000, Oleinik, Alexander wrote:
> > > When using qtest "in-process" communication, qtest_sendf directly
> > > calls
> > > a function in the server (qtest.c). Combining the contents of the
> > > subsequent socket_sends into the qtest_sendf, makes it so the
> > > server can
> > > immediately handle the command, without building a local buffer
> > > and
> > > waiting for a newline.
> > >
> > > Signed-off-by: Alexander Oleinik <alxndr@bu.edu<mailto:alxndr@bu.edu>>
> > > ---
> > >  tests/libqtest.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/tests/libqtest.c b/tests/libqtest.c
> > > index 19feea9e17..d770462869 100644
> > > --- a/tests/libqtest.c
> > > +++ b/tests/libqtest.c
> > > @@ -1086,9 +1086,7 @@ void qtest_bufwrite(QTestState *s, uint64_t
> > > addr, const void *data, size_t size)
> > >      gchar *bdata;
> > >
> > >      bdata = g_base64_encode(data, size);
> > > -    qtest_sendf(s, "b64write 0x%" PRIx64 " 0x%zx ", addr, size);
> > > -    socket_send(s->fd, bdata, strlen(bdata));
> > > -    socket_send(s->fd, "\n", 1);
> > > +    qtest_sendf(s, "b64write 0x%" PRIx64 " 0x%zx %s\n", addr,
> > > size, bdata);
> > >      qtest_rsp(s, 0);
> > >      g_free(bdata);
> > >  }
> > > --
> > > 2.23.0
> >
> > Cc John Snow, who added the b64write command.
> >
> > The downside to doing this is that sprintf-formatting needs to be
> > performed on the entire base64 buffer.  This slows things down
> > slightly
> > and a larger temporary buffer needs to be allocated, but I'm not
> > sure it
> > matters.
> >
>
> *struggles to remember*
>
> I guess I wanted something that had some space savings while
> maintaining
> some semblance of debuggability. This is almost certainly meant for
> AHCI
> tests where it's writing various patterns to large blocks of memory.
>
> I doubt I really measured the performance of it, but it seemed like
> the
> way to go for transferring medium amounts of data at the time via the
> qtest protocol.
>
> Looks like I am the only user of it, still:
>
> tests/ahci-test.c:    qtest_bufwrite(ahci->parent->qts, ptr, tx,
> bufsize);
> tests/ahci-test.c:    qtest_bufwrite(ahci->parent->qts, ptr, tx,
> bufsize);
> tests/libqos/ahci.c:        qtest_bufwrite(ahci->parent->qts, ptr,
> buffer, bufsize);
>
> The buffers can be quite large, so you might be re-buffering a decent
> amount of data from the sender now.
>
> 1, Are large transfers like this guaranteed to be atomic anyway? What
> kind of socket is it? we're probably eclipsing frame and packet sizes
> here.
>
> 2, I am not sure what being "atomic" affords us in terms of allowing
> the server to not wait for newlines, how does this change help?
>
> --js

I'm modifying qtest to allow the server and client to co-exist within
the same process (facilitating coverage-guided fuzzing). One of the
modifications is making qtest_sendf directly call a function in
qtest.c. All the other qtest commands are sent with a single
qtest_sendf call, so the qtest.c function could immediately call
qtest_process_command. This breaks if the command is sent with
different qtest_send/socket_send calls, as in b64write.

It should be simple to change qtest_server_inproc_recv (the qtest.c receiver) to wait for an "\n" prior to qtest_process_command, so I will probably do that and then normal(socket) qtest will keep the memory-reduction benefits of the non-"atomic" approach.

As a side note, would qtest_memwrite, also benefit from splitting up the send command?

    for (i = 0; i < size; i++) {
        sprintf(&enc[i * 2], "%02x", ptr[i]);
    }

    qtest_sendf(s, "write 0x%" PRIx64 " 0x%zx 0x%s\n", addr, size, enc);
    qtest_rsp(s, 0);
    g_free(enc);

[-- Attachment #2: Type: text/html, Size: 28204 bytes --]