Re: [PATCH V1 11/11] migration-test: test cpr-exec

[Peter Xu <peterx@redhat.com>]

On Tue, Sep 30, 2025 at 02:23:49PM -0400, Steven Sistare wrote:
> On 9/30/2025 1:08 PM, Peter Xu wrote:
> > On Fri, Sep 19, 2025 at 07:12:33AM -0700, Steve Sistare wrote:
> > > Add a test for the cpr-exec migration mode.
> > > 
> > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> > 
> > Looks good, only some nitpicks or pure questions below.
> > 
> > > ---
> > >   tests/qtest/migration/cpr-tests.c | 120 ++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 120 insertions(+)
> > > 
> > > diff --git a/tests/qtest/migration/cpr-tests.c b/tests/qtest/migration/cpr-tests.c
> > > index 5e764a6..f33af76 100644
> > > --- a/tests/qtest/migration/cpr-tests.c
> > > +++ b/tests/qtest/migration/cpr-tests.c
> > > @@ -110,6 +110,125 @@ static void test_mode_transfer_defer(void)
> > >       test_mode_transfer_common(true);
> > >   }
> > > +static void set_cpr_exec_args(QTestState *who, MigrateCommon *args)
> > > +{
> > > +    g_autofree char *qtest_from_args = NULL;
> > > +    g_autofree char *from_args = NULL;
> > > +    g_autofree char *to_args = NULL;
> > > +    g_autofree char *exec_args = NULL;
> > > +    g_auto(GStrv) argv = NULL;
> > > +    char *from_str, *src, *dst;
> > > +
> > > +    args->start.hide_stderr = false;    /* omit redirection word from args */
> > 
> > It's default off, right?  Could I request for some more explanations?
> 
> Yes, the default is false, so I will omit this line.  I will change it to
> an assertion. (IIRC when I first wrote this code 1-2 years ago, the cpr-exec
> test was a derivative of a precopy common test that set hide_stderr=true).
> 
> hide_stderr must be false when deriving cpr-exec arguments because of
> this code in framework.c:
> 
>     if (!getenv("QTEST_LOG") && args->hide_stderr) {
>         ignore_stderr = "2>/dev/null";
> 
> ignore_stderr is appended to the command line.  For cpr-exec the command line
> may not include redirection, because we pass it to execv(), not to the shell.

Please kindly add this rich comment above the assertion..

> 
> > Could we also set it in test_mode_exec() directly if needed?
> 
> Yes, one can set hide_stderr when launching the source VM.
> 
> > > +    migrate_args(&from_args, &to_args, args->listen_uri, &args->start);
> > > +    qtest_from_args = qtest_qemu_args(from_args);
> > > +
> > > +    /* De-dup spaces so argv does not contain empty strings */
> > > +    from_str = src = dst = g_strstrip(qtest_from_args);
> > > +    do {
> > > +        if (*src != ' ' || src[-1] != ' ') {
> > > +            *dst++ = *src;
> > > +        }
> > > +    } while (*src++);
> > 
> > Pure ask.. when will empty string be present?
> 
> migrate_args() format strings "%s %s %s" produce "   " when the arguments
> are empty strings.  Then g_strsplit("   ") would produce an array of 3
> empty strings.

... and here too.

Thanks,

-- 
Peter Xu