[Qemu-devel] [PATCH 0/2] tests: multifd migration test scenarios

[Qemu-devel] [PATCH 0/2] tests: multifd migration test scenarios

[Balamuruhan S]

This patchset adds test scenarios for precopy multifd migration
using fd and unix.

Also I am working on adding scenarios in tp-qemu using Avocado-VT
and observed `Segmentation fault` with recently added test scenario
using fd where as unix and tcp got PASS, 

https://github.com/autotest/tp-qemu/pull/1433

so thought to recreate using qtest but it works fine here, I will debug
more whether it is a test environment issue or real bug meanwhile.

QTest results:
# QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 \
QTEST_QEMU_IMG=qemu-img MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( \
${RANDOM:-0} % 255 + 1))} gtester -k --verbose -m=quick \
tests/migration-test
TEST: tests/migration-test... (pid=23465)
  /ppc64/migration/deprecated: OK
  /ppc64/migration/bad_dest: OK
  /ppc64/migration/postcopy/unix: OK
  /ppc64/migration/precopy/unix: OK
  /ppc64/migration/multifd/unix: OK
  /ppc64/migration/multifd/fd: OK
PASS: tests/migration-test

Please help review,

-- Bala

Balamuruhan S (2):
  tests: add precopy multifd migration test
  tests: add precopy multifd migration with fd protocol

 tests/migration-test.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] tests: add precopy multifd migration test with unix

[Balamuruhan S]

multifd migration feature is in with commit: 35374cbdff, this patch
adds test for it using unix socket.

Signed-off-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
---
 tests/migration-test.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 3a85446f95..e2434e70ea 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -619,6 +619,36 @@ static void test_precopy_unix(void)
     g_free(uri);
 }
 
+static void test_multifd_unix(void)
+{
+    char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+    QTestState *from, *to;
+    test_migrate_start(&from, &to, uri, false);
+
+    /* set multifd capability on source and target */
+    migrate_set_capability(from, "x-multifd", "true");
+    migrate_set_capability(to, "x-multifd", "true");
+
+    /* Wait for the first serial output from the source */
+    wait_for_serial("src_serial");
+
+    /* perform migration */
+    migrate(from, uri);
+
+    wait_for_migration_pass(from);
+
+    if (!got_stop) {
+        qtest_qmp_eventwait(from, "STOP");
+    }
+
+    qtest_qmp_eventwait(to, "RESUME");
+
+    wait_for_serial("dest_serial");
+    wait_for_migration_complete(from);
+    test_migrate_end(from, to, true);
+    g_free(uri);
+}
+
 int main(int argc, char **argv)
 {
     char template[] = "/tmp/migration-test-XXXXXX";
@@ -642,6 +672,7 @@ int main(int argc, char **argv)
     qtest_add_func("/migration/deprecated", test_deprecated);
     qtest_add_func("/migration/bad_dest", test_baddest);
     qtest_add_func("/migration/precopy/unix", test_precopy_unix);
+    qtest_add_func("/migration/multifd/unix", test_multifd_unix);
 
     ret = g_test_run();
 
-- 
2.14.3

[Qemu-devel] [PATCH 2/2] tests: add precopy multifd migration with fd protocol

[Balamuruhan S]

This patch adds test for multifd migration feature with fd protocol.

Signed-off-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
---
 tests/migration-test.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index e2434e70ea..29ede3810d 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -10,6 +10,7 @@
  *
  */
 
+#include <unistd.h>
 #include "qemu/osdep.h"
 
 #include "libqtest.h"
@@ -202,6 +203,17 @@ static void read_blocktime(QTestState *who)
     qobject_unref(rsp);
 }
 
+static void getfd(QTestState *who, const char *fdname, int fd)
+{
+    QDict *rsp;
+    gchar *cmd = g_strdup_printf("{ 'execute': 'getfd',"
+                                 "'arguments': { '%s': '%d'} }", fdname, fd);
+    rsp = wait_command(who, cmd);
+    g_assert(qdict_haskey(rsp, "return"));
+    g_free(cmd);
+    qobject_unref(rsp);
+}
+
 static void wait_for_migration_complete(QTestState *who)
 {
     while (true) {
@@ -619,6 +631,50 @@ static void test_precopy_unix(void)
     g_free(uri);
 }
 
+static void test_multifd_fd(void)
+{
+    int fd[2];
+
+    /* create pipe */
+    if (pipe(fd) == -1)
+        g_test_message("Skipping test: Unable to create pipe");
+        return;
+
+    /* set uri in target with read fd */
+    char *uri = g_strdup_printf("fd:%d", fd[0]);
+    QTestState *from, *to;
+    test_migrate_start(&from, &to, uri, false);
+
+    /* Receive a write file descriptor for source using getfd */
+    getfd(from, "srcfd", fd[1]);
+
+    /* set multifd capability on source and target */
+    migrate_set_capability(from, "x-multifd", "true");
+    migrate_set_capability(to, "x-multifd", "true");
+
+    /* Wait for the first serial output from the source */
+    wait_for_serial("src_serial");
+
+    /* perform migration */
+    migrate(from, uri);
+
+    wait_for_migration_pass(from);
+
+    if (!got_stop) {
+        qtest_qmp_eventwait(from, "STOP");
+    }
+    qtest_qmp_eventwait(to, "RESUME");
+
+    wait_for_serial("dest_serial");
+    wait_for_migration_complete(from);
+    test_migrate_end(from, to, true);
+    g_free(uri);
+
+    /* close the fds */
+    close(fd[0]);
+    close(fd[1]);
+}
+
 static void test_multifd_unix(void)
 {
     char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
@@ -673,6 +729,7 @@ int main(int argc, char **argv)
     qtest_add_func("/migration/bad_dest", test_baddest);
     qtest_add_func("/migration/precopy/unix", test_precopy_unix);
     qtest_add_func("/migration/multifd/unix", test_multifd_unix);
+    qtest_add_func("/migration/multifd/fd", test_multifd_fd);
 
     ret = g_test_run();
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH 0/2] tests: multifd migration test scenarios

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180705080017.31123-1-bala24@linux.vnet.ibm.com
Subject: [Qemu-devel]  [PATCH 0/2] tests: multifd migration test scenarios

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
12e7e89183 tests: add precopy multifd migration with fd protocol
72f2caa64b tests: add precopy multifd migration test with unix

=== OUTPUT BEGIN ===
Checking PATCH 1/2: tests: add precopy multifd migration test with unix...
Checking PATCH 2/2: tests: add precopy multifd migration with fd protocol...
ERROR: braces {} are necessary for all arms of this statement
#50: FILE: tests/migration-test.c:639:
+    if (pipe(fd) == -1)
[...]

total: 1 errors, 0 warnings, 81 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH 1/2] tests: add precopy multifd migration test with unix

[Peter Xu]

On Thu, Jul 05, 2018 at 01:30:16PM +0530, Balamuruhan S wrote:
> multifd migration feature is in with commit: 35374cbdff, this patch
> adds test for it using unix socket.
> 
> Signed-off-by: Balamuruhan S <bala24@linux.vnet.ibm.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/2] tests: add precopy multifd migration with fd protocol

[Peter Xu]

On Thu, Jul 05, 2018 at 01:30:17PM +0530, Balamuruhan S wrote:
> This patch adds test for multifd migration feature with fd protocol.
> 
> Signed-off-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
> ---
>  tests/migration-test.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index e2434e70ea..29ede3810d 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -10,6 +10,7 @@
>   *
>   */
>  
> +#include <unistd.h>
>  #include "qemu/osdep.h"
>  
>  #include "libqtest.h"
> @@ -202,6 +203,17 @@ static void read_blocktime(QTestState *who)
>      qobject_unref(rsp);
>  }
>  
> +static void getfd(QTestState *who, const char *fdname, int fd)
> +{
> +    QDict *rsp;
> +    gchar *cmd = g_strdup_printf("{ 'execute': 'getfd',"
> +                                 "'arguments': { '%s': '%d'} }", fdname, fd);
> +    rsp = wait_command(who, cmd);

AFAIU Libvirt passes fds to QEMU via the QMP channel using SCM_RIGHTS.
Here we directly specified the fd number. Could you help explain a bit
on how it worked?

> +    g_assert(qdict_haskey(rsp, "return"));
> +    g_free(cmd);
> +    qobject_unref(rsp);
> +}
> +
>  static void wait_for_migration_complete(QTestState *who)
>  {
>      while (true) {
> @@ -619,6 +631,50 @@ static void test_precopy_unix(void)
>      g_free(uri);
>  }
>  
> +static void test_multifd_fd(void)
> +{
> +    int fd[2];
> +
> +    /* create pipe */
> +    if (pipe(fd) == -1)
> +        g_test_message("Skipping test: Unable to create pipe");
> +        return;
> +
> +    /* set uri in target with read fd */
> +    char *uri = g_strdup_printf("fd:%d", fd[0]);
> +    QTestState *from, *to;
> +    test_migrate_start(&from, &to, uri, false);
> +
> +    /* Receive a write file descriptor for source using getfd */
> +    getfd(from, "srcfd", fd[1]);
> +
> +    /* set multifd capability on source and target */
> +    migrate_set_capability(from, "x-multifd", "true");
> +    migrate_set_capability(to, "x-multifd", "true");
> +
> +    /* Wait for the first serial output from the source */
> +    wait_for_serial("src_serial");
> +
> +    /* perform migration */
> +    migrate(from, uri);

I'm not sure I understand correctly here, but I feel like we should
use the fd[1] or anything that was bound to fd[1] on source side to do
the migration rather than fd[0]?  Please feel free to correct me..

Regards,

> +
> +    wait_for_migration_pass(from);
> +
> +    if (!got_stop) {
> +        qtest_qmp_eventwait(from, "STOP");
> +    }
> +    qtest_qmp_eventwait(to, "RESUME");
> +
> +    wait_for_serial("dest_serial");
> +    wait_for_migration_complete(from);
> +    test_migrate_end(from, to, true);
> +    g_free(uri);
> +
> +    /* close the fds */
> +    close(fd[0]);
> +    close(fd[1]);
> +}
> +
>  static void test_multifd_unix(void)
>  {
>      char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> @@ -673,6 +729,7 @@ int main(int argc, char **argv)
>      qtest_add_func("/migration/bad_dest", test_baddest);
>      qtest_add_func("/migration/precopy/unix", test_precopy_unix);
>      qtest_add_func("/migration/multifd/unix", test_multifd_unix);
> +    qtest_add_func("/migration/multifd/fd", test_multifd_fd);
>  
>      ret = g_test_run();
>  
> -- 
> 2.14.3
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/2] tests: add precopy multifd migration with fd protocol

[Balamuruhan S]

On Thu, Jul 05, 2018 at 05:26:09PM +0800, Peter Xu wrote:
> On Thu, Jul 05, 2018 at 01:30:17PM +0530, Balamuruhan S wrote:
> > This patch adds test for multifd migration feature with fd protocol.
> > 
> > Signed-off-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
> > ---
> >  tests/migration-test.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> > 
> > diff --git a/tests/migration-test.c b/tests/migration-test.c
> > index e2434e70ea..29ede3810d 100644
> > --- a/tests/migration-test.c
> > +++ b/tests/migration-test.c
> > @@ -10,6 +10,7 @@
> >   *
> >   */
> >  
> > +#include <unistd.h>
> >  #include "qemu/osdep.h"
> >  
> >  #include "libqtest.h"
> > @@ -202,6 +203,17 @@ static void read_blocktime(QTestState *who)
> >      qobject_unref(rsp);
> >  }
> >  
> > +static void getfd(QTestState *who, const char *fdname, int fd)
> > +{
> > +    QDict *rsp;
> > +    gchar *cmd = g_strdup_printf("{ 'execute': 'getfd',"
> > +                                 "'arguments': { '%s': '%d'} }", fdname, fd);
> > +    rsp = wait_command(who, cmd);
> 
> AFAIU Libvirt passes fds to QEMU via the QMP channel using SCM_RIGHTS.
> Here we directly specified the fd number. Could you help explain a bit
> on how it worked?

Thank you Peter for reviewing the patch.

so I explored on pipe() system call, that could create channel for inter
process communication. It gives us 2 fds one for read (I thought
we can use it for target qemu process) and one for write (we can use it
for source qemu process). I tried the example snippet from man page so
I thought we can use it in our migration Qtest.

Please correct my work if it doesn't make sense.

> 
> > +    g_assert(qdict_haskey(rsp, "return"));
> > +    g_free(cmd);
> > +    qobject_unref(rsp);
> > +}
> > +
> >  static void wait_for_migration_complete(QTestState *who)
> >  {
> >      while (true) {
> > @@ -619,6 +631,50 @@ static void test_precopy_unix(void)
> >      g_free(uri);
> >  }
> >  
> > +static void test_multifd_fd(void)
> > +{
> > +    int fd[2];
> > +
> > +    /* create pipe */
> > +    if (pipe(fd) == -1)
> > +        g_test_message("Skipping test: Unable to create pipe");
> > +        return;
> > +
> > +    /* set uri in target with read fd */
> > +    char *uri = g_strdup_printf("fd:%d", fd[0]);
> > +    QTestState *from, *to;
> > +    test_migrate_start(&from, &to, uri, false);
> > +
> > +    /* Receive a write file descriptor for source using getfd */
> > +    getfd(from, "srcfd", fd[1]);
> > +
> > +    /* set multifd capability on source and target */
> > +    migrate_set_capability(from, "x-multifd", "true");
> > +    migrate_set_capability(to, "x-multifd", "true");
> > +
> > +    /* Wait for the first serial output from the source */
> > +    wait_for_serial("src_serial");
> > +
> > +    /* perform migration */
> > +    migrate(from, uri);
> 
> I'm not sure I understand correctly here, but I feel like we should
> use the fd[1] or anything that was bound to fd[1] on source side to do
> the migration rather than fd[0]?  Please feel free to correct me..
> 

fd[0] - read channel so we should use it for target (-incoming fd:fd[0])
fd[1] - write channel so we shouls use it for source to transfer/write
and for it we need to assign using `getfd` monitor command on source.

> Regards,
> 
> > +
> > +    wait_for_migration_pass(from);
> > +
> > +    if (!got_stop) {
> > +        qtest_qmp_eventwait(from, "STOP");
> > +    }
> > +    qtest_qmp_eventwait(to, "RESUME");
> > +
> > +    wait_for_serial("dest_serial");
> > +    wait_for_migration_complete(from);
> > +    test_migrate_end(from, to, true);
> > +    g_free(uri);
> > +
> > +    /* close the fds */
> > +    close(fd[0]);
> > +    close(fd[1]);

After studying manual example program, we should close the fds
before we start migration like we use for O_CLOSEXEC on fd.
so I will work on resend the patch and fixing style issue caught by
patchew (sorry for it).

-- Bala
> > +}
> > +
> >  static void test_multifd_unix(void)
> >  {
> >      char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> > @@ -673,6 +729,7 @@ int main(int argc, char **argv)
> >      qtest_add_func("/migration/bad_dest", test_baddest);
> >      qtest_add_func("/migration/precopy/unix", test_precopy_unix);
> >      qtest_add_func("/migration/multifd/unix", test_multifd_unix);
> > +    qtest_add_func("/migration/multifd/fd", test_multifd_fd);
> >  
> >      ret = g_test_run();
> >  
> > -- 
> > 2.14.3
> > 
> 
> -- 
> Peter Xu
> 

Re: [Qemu-devel] [PATCH 2/2] tests: add precopy multifd migration with fd protocol

[Peter Xu]

On Thu, Jul 05, 2018 at 06:22:31PM +0530, Balamuruhan S wrote:
> On Thu, Jul 05, 2018 at 05:26:09PM +0800, Peter Xu wrote:
> > On Thu, Jul 05, 2018 at 01:30:17PM +0530, Balamuruhan S wrote:
> > > This patch adds test for multifd migration feature with fd protocol.
> > > 
> > > Signed-off-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
> > > ---
> > >  tests/migration-test.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 57 insertions(+)
> > > 
> > > diff --git a/tests/migration-test.c b/tests/migration-test.c
> > > index e2434e70ea..29ede3810d 100644
> > > --- a/tests/migration-test.c
> > > +++ b/tests/migration-test.c
> > > @@ -10,6 +10,7 @@
> > >   *
> > >   */
> > >  
> > > +#include <unistd.h>
> > >  #include "qemu/osdep.h"
> > >  
> > >  #include "libqtest.h"
> > > @@ -202,6 +203,17 @@ static void read_blocktime(QTestState *who)
> > >      qobject_unref(rsp);
> > >  }
> > >  
> > > +static void getfd(QTestState *who, const char *fdname, int fd)
> > > +{
> > > +    QDict *rsp;
> > > +    gchar *cmd = g_strdup_printf("{ 'execute': 'getfd',"
> > > +                                 "'arguments': { '%s': '%d'} }", fdname, fd);
> > > +    rsp = wait_command(who, cmd);
> > 
> > AFAIU Libvirt passes fds to QEMU via the QMP channel using SCM_RIGHTS.
> > Here we directly specified the fd number. Could you help explain a bit
> > on how it worked?
> 
> Thank you Peter for reviewing the patch.
> 
> so I explored on pipe() system call, that could create channel for inter
> process communication. It gives us 2 fds one for read (I thought
> we can use it for target qemu process) and one for write (we can use it
> for source qemu process). I tried the example snippet from man page so
> I thought we can use it in our migration Qtest.
> 
> Please correct my work if it doesn't make sense.

Yeah the pipe() usage should be fine - though it's the way to pass it
to QEMU that I'm not sure about.

Ok I gave it a shot with your tests on my host and I got this
actually:

  $ QTEST_QEMU_BINARY=./x86_64-softmmu/qemu-system-x86_64 ./tests/migration-test -p /x86_64/migration/multifd/fd
  /x86_64/migration/multifd/fd: **
  ERROR:/home/peterx/git/qemu/tests/migration-test.c:212:getfd: assertion failed: (qdict_haskey(rsp, "return"))
  Aborted (core dumped)
  $ QTEST_QEMU_BINARY=./x86_64-softmmu/qemu-system-x86_64 QTEST_LOG=1 ./tests/migration-test -p /x86_64/migration/multifd/fd
  /x86_64/migration/multifd/fd: [I 1530851360.669526] OPENED
  [R +0.037850] endianness
  [S +0.037870] OK little
  {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-2164-g20aa87a802"}, "capabilities": []}}{"execute": "qmp_capabilities"}
  
  {"return": {}}[I 1530851360.754972] OPENED
  [R +0.055719] endianness
  [S +0.055737] OK little
  {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-2164-g20aa87a802"}, "capabilities": []}}{"execute": "qmp_capabilities"}
  
  {"return": {}}{"execute": "getfd", "arguments": {"srcfd": "5"}}
  
  {"error": {"class": "GenericError", "desc": "Parameter 'fdname' is missing"}}**
  ERROR:/home/peterx/git/qemu/tests/migration-test.c:212:getfd: assertion failed: (qdict_haskey(rsp, "return"))
  [I +0.058378] CLOSED
  [I +0.156930] CLOSED
  Aborted (core dumped)

So it seems that the "getfd" command will only take one parameter
"fdname", then the fd should be passed in by UNIX socket magic.  That
should match with what I understand it.  I'm not sure why it worked on
your host.

> 
> > 
> > > +    g_assert(qdict_haskey(rsp, "return"));
> > > +    g_free(cmd);
> > > +    qobject_unref(rsp);
> > > +}
> > > +
> > >  static void wait_for_migration_complete(QTestState *who)
> > >  {
> > >      while (true) {
> > > @@ -619,6 +631,50 @@ static void test_precopy_unix(void)
> > >      g_free(uri);
> > >  }
> > >  
> > > +static void test_multifd_fd(void)
> > > +{
> > > +    int fd[2];
> > > +
> > > +    /* create pipe */
> > > +    if (pipe(fd) == -1)
> > > +        g_test_message("Skipping test: Unable to create pipe");
> > > +        return;
> > > +
> > > +    /* set uri in target with read fd */
> > > +    char *uri = g_strdup_printf("fd:%d", fd[0]);
> > > +    QTestState *from, *to;
> > > +    test_migrate_start(&from, &to, uri, false);
> > > +
> > > +    /* Receive a write file descriptor for source using getfd */
> > > +    getfd(from, "srcfd", fd[1]);
> > > +
> > > +    /* set multifd capability on source and target */
> > > +    migrate_set_capability(from, "x-multifd", "true");
> > > +    migrate_set_capability(to, "x-multifd", "true");
> > > +
> > > +    /* Wait for the first serial output from the source */
> > > +    wait_for_serial("src_serial");
> > > +
> > > +    /* perform migration */
> > > +    migrate(from, uri);
> > 
> > I'm not sure I understand correctly here, but I feel like we should
> > use the fd[1] or anything that was bound to fd[1] on source side to do
> > the migration rather than fd[0]?  Please feel free to correct me..
> > 
> 
> fd[0] - read channel so we should use it for target (-incoming fd:fd[0])
> fd[1] - write channel so we shouls use it for source to transfer/write
> and for it we need to assign using `getfd` monitor command on source.

Yeah again the logic should be fine, though we might need to implement
the SCM_RIGHTS magic here, like what we did in, e.g.,
qio_channel_socket_writev() in QEMU.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/2] tests: add precopy multifd migration with fd protocol

[Balamuruhan S]

On Fri, Jul 06, 2018 at 12:37:40PM +0800, Peter Xu wrote:
> On Thu, Jul 05, 2018 at 06:22:31PM +0530, Balamuruhan S wrote:
> > On Thu, Jul 05, 2018 at 05:26:09PM +0800, Peter Xu wrote:
> > > On Thu, Jul 05, 2018 at 01:30:17PM +0530, Balamuruhan S wrote:
> > > > This patch adds test for multifd migration feature with fd protocol.
> > > > 
> > > > Signed-off-by: Balamuruhan S <bala24@linux.vnet.ibm.com>
> > > > ---
> > > >  tests/migration-test.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 57 insertions(+)
> > > > 
> > > > diff --git a/tests/migration-test.c b/tests/migration-test.c
> > > > index e2434e70ea..29ede3810d 100644
> > > > --- a/tests/migration-test.c
> > > > +++ b/tests/migration-test.c
> > > > @@ -10,6 +10,7 @@
> > > >   *
> > > >   */
> > > >  
> > > > +#include <unistd.h>
> > > >  #include "qemu/osdep.h"
> > > >  
> > > >  #include "libqtest.h"
> > > > @@ -202,6 +203,17 @@ static void read_blocktime(QTestState *who)
> > > >      qobject_unref(rsp);
> > > >  }
> > > >  
> > > > +static void getfd(QTestState *who, const char *fdname, int fd)
> > > > +{
> > > > +    QDict *rsp;
> > > > +    gchar *cmd = g_strdup_printf("{ 'execute': 'getfd',"
> > > > +                                 "'arguments': { '%s': '%d'} }", fdname, fd);
> > > > +    rsp = wait_command(who, cmd);
> > > 
> > > AFAIU Libvirt passes fds to QEMU via the QMP channel using SCM_RIGHTS.
> > > Here we directly specified the fd number. Could you help explain a bit
> > > on how it worked?
> > 
> > Thank you Peter for reviewing the patch.
> > 
> > so I explored on pipe() system call, that could create channel for inter
> > process communication. It gives us 2 fds one for read (I thought
> > we can use it for target qemu process) and one for write (we can use it
> > for source qemu process). I tried the example snippet from man page so
> > I thought we can use it in our migration Qtest.
> > 
> > Please correct my work if it doesn't make sense.
> 
> Yeah the pipe() usage should be fine - though it's the way to pass it
> to QEMU that I'm not sure about.
> 
> Ok I gave it a shot with your tests on my host and I got this
> actually:
> 
>   $ QTEST_QEMU_BINARY=./x86_64-softmmu/qemu-system-x86_64 ./tests/migration-test -p /x86_64/migration/multifd/fd
>   /x86_64/migration/multifd/fd: **
>   ERROR:/home/peterx/git/qemu/tests/migration-test.c:212:getfd: assertion failed: (qdict_haskey(rsp, "return"))
>   Aborted (core dumped)
>   $ QTEST_QEMU_BINARY=./x86_64-softmmu/qemu-system-x86_64 QTEST_LOG=1 ./tests/migration-test -p /x86_64/migration/multifd/fd
>   /x86_64/migration/multifd/fd: [I 1530851360.669526] OPENED
>   [R +0.037850] endianness
>   [S +0.037870] OK little
>   {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-2164-g20aa87a802"}, "capabilities": []}}{"execute": "qmp_capabilities"}
>   
>   {"return": {}}[I 1530851360.754972] OPENED
>   [R +0.055719] endianness
>   [S +0.055737] OK little
>   {"QMP": {"version": {"qemu": {"micro": 50, "minor": 12, "major": 2}, "package": "v2.12.0-2164-g20aa87a802"}, "capabilities": []}}{"execute": "qmp_capabilities"}
>   
>   {"return": {}}{"execute": "getfd", "arguments": {"srcfd": "5"}}
>   
>   {"error": {"class": "GenericError", "desc": "Parameter 'fdname' is missing"}}**
>   ERROR:/home/peterx/git/qemu/tests/migration-test.c:212:getfd: assertion failed: (qdict_haskey(rsp, "return"))
>   [I +0.058378] CLOSED
>   [I +0.156930] CLOSED
>   Aborted (core dumped)

Okay, I think this is what I am facing when I run it in tp-qemu which I
mentioned in cover letter, I will work on it to make it work in right way.

> 
> So it seems that the "getfd" command will only take one parameter
> "fdname", then the fd should be passed in by UNIX socket magic.  That
> should match with what I understand it.  I'm not sure why it worked on
> your host.

fine, I will use the getfd correctly as per your suggestion, I couldn't hit error,
it was silent false pass.

> 
> > 
> > > 
> > > > +    g_assert(qdict_haskey(rsp, "return"));
> > > > +    g_free(cmd);
> > > > +    qobject_unref(rsp);
> > > > +}
> > > > +
> > > >  static void wait_for_migration_complete(QTestState *who)
> > > >  {
> > > >      while (true) {
> > > > @@ -619,6 +631,50 @@ static void test_precopy_unix(void)
> > > >      g_free(uri);
> > > >  }
> > > >  
> > > > +static void test_multifd_fd(void)
> > > > +{
> > > > +    int fd[2];
> > > > +
> > > > +    /* create pipe */
> > > > +    if (pipe(fd) == -1)
> > > > +        g_test_message("Skipping test: Unable to create pipe");
> > > > +        return;
> > > > +
> > > > +    /* set uri in target with read fd */
> > > > +    char *uri = g_strdup_printf("fd:%d", fd[0]);
> > > > +    QTestState *from, *to;
> > > > +    test_migrate_start(&from, &to, uri, false);
> > > > +
> > > > +    /* Receive a write file descriptor for source using getfd */
> > > > +    getfd(from, "srcfd", fd[1]);
> > > > +
> > > > +    /* set multifd capability on source and target */
> > > > +    migrate_set_capability(from, "x-multifd", "true");
> > > > +    migrate_set_capability(to, "x-multifd", "true");
> > > > +
> > > > +    /* Wait for the first serial output from the source */
> > > > +    wait_for_serial("src_serial");
> > > > +
> > > > +    /* perform migration */
> > > > +    migrate(from, uri);
> > > 
> > > I'm not sure I understand correctly here, but I feel like we should
> > > use the fd[1] or anything that was bound to fd[1] on source side to do
> > > the migration rather than fd[0]?  Please feel free to correct me..
> > > 
> > 
> > fd[0] - read channel so we should use it for target (-incoming fd:fd[0])
> > fd[1] - write channel so we shouls use it for source to transfer/write
> > and for it we need to assign using `getfd` monitor command on source.
> 
> Yeah again the logic should be fine, though we might need to implement
> the SCM_RIGHTS magic here, like what we did in, e.g.,
> qio_channel_socket_writev() in QEMU.

:+1: sure, I will work on it, Thank you Peter for your time and help.

-- Bala
> 
> Regards,
> 
> -- 
> Peter Xu
>