[PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Jaehoon Kim]

When the source VM attempts to connect to the destination VM's Unix
domain socket(cpr.sock) during CPR transfer, the socket file might not
yet be exist if the destination side hasn't completed the bind
operation. This can lead to connection failures when running tests with
the qtest framework.

To address this, add cpr_validate_socket_path(), which wait for the
socket file to appear. This avoids intermittent qtest failures caused by
early connection attempts.

Signed-off-by: Jaehoon Kim <jhkim@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
---
 include/migration/cpr.h  |  1 +
 migration/cpr-transfer.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/include/migration/cpr.h b/include/migration/cpr.h
index 7561fc75ad..cc9384b4f9 100644
--- a/include/migration/cpr.h
+++ b/include/migration/cpr.h
@@ -23,6 +23,7 @@ MigMode cpr_get_incoming_mode(void);
 void cpr_set_incoming_mode(MigMode mode);
 bool cpr_is_incoming(void);
 
+bool cpr_validate_socket_path(const char *path, Error **errp);
 int cpr_state_save(MigrationChannel *channel, Error **errp);
 int cpr_state_load(MigrationChannel *channel, Error **errp);
 void cpr_state_close(void);
diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
index e1f140359c..3088ed323f 100644
--- a/migration/cpr-transfer.c
+++ b/migration/cpr-transfer.c
@@ -17,6 +17,33 @@
 #include "migration/vmstate.h"
 #include "trace.h"
 
+#define CPR_MAX_RETRIES     50     /* Retry for up to 5 seconds */
+#define CPR_RETRY_DELAY_US  100000 /* 100 ms per retry */
+
+bool cpr_validate_socket_path(const char *path, Error **errp)
+{
+    struct stat st;
+    int retries = CPR_MAX_RETRIES;
+
+    do {
+        if (!stat(path, &st) && S_ISSOCK(st.st_mode)) {
+            return true;
+        }
+
+        if (errno == ENOENT) {
+            usleep(CPR_RETRY_DELAY_US);
+        } else {
+            error_setg_errno(errp, errno,
+                "Unable to check status of socket path '%s'", path);
+            return false;
+        }
+    } while (--retries > 0);
+
+    error_setg(errp, "Socket path '%s' not found after %d retries",
+                                            path, CPR_MAX_RETRIES);
+    return false;
+}
+
 QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp)
 {
     MigrationAddress *addr = channel->addr;
@@ -28,6 +55,14 @@ QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp)
         QIOChannel *ioc = QIO_CHANNEL(sioc);
         SocketAddress *saddr = &addr->u.socket;
 
+        /*
+         * Verify that the cpr.sock Unix domain socket file exists and is ready
+         * before proceeding with the connection.
+         */
+        if (!cpr_validate_socket_path(addr->u.socket.u.q_unix.path, errp)) {
+            return NULL;
+        }
+
         if (qio_channel_socket_connect_sync(sioc, saddr, errp) < 0) {
             return NULL;
         }
-- 
2.49.0

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Fabiano Rosas]

Jaehoon Kim <jhkim@linux.ibm.com> writes:

> When the source VM attempts to connect to the destination VM's Unix
> domain socket(cpr.sock) during CPR transfer, the socket file might not
> yet be exist if the destination side hasn't completed the bind
> operation. This can lead to connection failures when running tests with
> the qtest framework.
>

Could you provide us the output of qtest in this case? Are you simply running
make check or something else?

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Daniel P. Berrangé]

On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
> When the source VM attempts to connect to the destination VM's Unix
> domain socket(cpr.sock) during CPR transfer, the socket file might not
> yet be exist if the destination side hasn't completed the bind
> operation. This can lead to connection failures when running tests with
> the qtest framework.

This sounds like a flawed test impl to me - whatever is initiating
the cpr operation on the source has done so prematurely - it should
ensure the dest is ready before starting the operation.

> To address this, add cpr_validate_socket_path(), which wait for the
> socket file to appear. This avoids intermittent qtest failures caused by
> early connection attempts.

IMHO it is dubious to special case cpr in this way.

> 
> Signed-off-by: Jaehoon Kim <jhkim@linux.ibm.com>
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
> ---
>  include/migration/cpr.h  |  1 +
>  migration/cpr-transfer.c | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
> index 7561fc75ad..cc9384b4f9 100644
> --- a/include/migration/cpr.h
> +++ b/include/migration/cpr.h
> @@ -23,6 +23,7 @@ MigMode cpr_get_incoming_mode(void);
>  void cpr_set_incoming_mode(MigMode mode);
>  bool cpr_is_incoming(void);
>  
> +bool cpr_validate_socket_path(const char *path, Error **errp);
>  int cpr_state_save(MigrationChannel *channel, Error **errp);
>  int cpr_state_load(MigrationChannel *channel, Error **errp);
>  void cpr_state_close(void);
> diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
> index e1f140359c..3088ed323f 100644
> --- a/migration/cpr-transfer.c
> +++ b/migration/cpr-transfer.c
> @@ -17,6 +17,33 @@
>  #include "migration/vmstate.h"
>  #include "trace.h"
>  
> +#define CPR_MAX_RETRIES     50     /* Retry for up to 5 seconds */
> +#define CPR_RETRY_DELAY_US  100000 /* 100 ms per retry */
> +
> +bool cpr_validate_socket_path(const char *path, Error **errp)
> +{
> +    struct stat st;
> +    int retries = CPR_MAX_RETRIES;
> +
> +    do {
> +        if (!stat(path, &st) && S_ISSOCK(st.st_mode)) {
> +            return true;
> +        }
> +
> +        if (errno == ENOENT) {
> +            usleep(CPR_RETRY_DELAY_US);
> +        } else {
> +            error_setg_errno(errp, errno,
> +                "Unable to check status of socket path '%s'", path);
> +            return false;
> +        }
> +    } while (--retries > 0);
> +
> +    error_setg(errp, "Socket path '%s' not found after %d retries",
> +                                            path, CPR_MAX_RETRIES);
> +    return false;
> +}
> +
>  QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp)
>  {
>      MigrationAddress *addr = channel->addr;
> @@ -28,6 +55,14 @@ QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp)
>          QIOChannel *ioc = QIO_CHANNEL(sioc);
>          SocketAddress *saddr = &addr->u.socket;
>  
> +        /*
> +         * Verify that the cpr.sock Unix domain socket file exists and is ready
> +         * before proceeding with the connection.
> +         */
> +        if (!cpr_validate_socket_path(addr->u.socket.u.q_unix.path, errp)) {
> +            return NULL;
> +        }
> +
>          if (qio_channel_socket_connect_sync(sioc, saddr, errp) < 0) {
>              return NULL;
>          }
> -- 
> 2.49.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Steven Sistare]

On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
> On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
>> When the source VM attempts to connect to the destination VM's Unix
>> domain socket(cpr.sock) during CPR transfer, the socket file might not
>> yet be exist if the destination side hasn't completed the bind
>> operation. This can lead to connection failures when running tests with
>> the qtest framework.
> 
> This sounds like a flawed test impl to me - whatever is initiating
> the cpr operation on the source has done so prematurely - it should
> ensure the dest is ready before starting the operation.
> 
>> To address this, add cpr_validate_socket_path(), which wait for the
>> socket file to appear. This avoids intermittent qtest failures caused by
>> early connection attempts.
> 
> IMHO it is dubious to special case cpr in this way.

I agree with Daniel, and unfortunately it is not just a test issue;
every management framework that supports cpr-transfer must add logic to
wait for the cpr socket to appear in the target before proceeding.

This is analogous to waiting for the monitor socket to appear before
connecting to it.

- Steve

>> Signed-off-by: Jaehoon Kim <jhkim@linux.ibm.com>
>> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
>> ---
>>   include/migration/cpr.h  |  1 +
>>   migration/cpr-transfer.c | 35 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 36 insertions(+)
>>
>> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
>> index 7561fc75ad..cc9384b4f9 100644
>> --- a/include/migration/cpr.h
>> +++ b/include/migration/cpr.h
>> @@ -23,6 +23,7 @@ MigMode cpr_get_incoming_mode(void);
>>   void cpr_set_incoming_mode(MigMode mode);
>>   bool cpr_is_incoming(void);
>>   
>> +bool cpr_validate_socket_path(const char *path, Error **errp);
>>   int cpr_state_save(MigrationChannel *channel, Error **errp);
>>   int cpr_state_load(MigrationChannel *channel, Error **errp);
>>   void cpr_state_close(void);
>> diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
>> index e1f140359c..3088ed323f 100644
>> --- a/migration/cpr-transfer.c
>> +++ b/migration/cpr-transfer.c
>> @@ -17,6 +17,33 @@
>>   #include "migration/vmstate.h"
>>   #include "trace.h"
>>   
>> +#define CPR_MAX_RETRIES     50     /* Retry for up to 5 seconds */
>> +#define CPR_RETRY_DELAY_US  100000 /* 100 ms per retry */
>> +
>> +bool cpr_validate_socket_path(const char *path, Error **errp)
>> +{
>> +    struct stat st;
>> +    int retries = CPR_MAX_RETRIES;
>> +
>> +    do {
>> +        if (!stat(path, &st) && S_ISSOCK(st.st_mode)) {
>> +            return true;
>> +        }
>> +
>> +        if (errno == ENOENT) {
>> +            usleep(CPR_RETRY_DELAY_US);
>> +        } else {
>> +            error_setg_errno(errp, errno,
>> +                "Unable to check status of socket path '%s'", path);
>> +            return false;
>> +        }
>> +    } while (--retries > 0);
>> +
>> +    error_setg(errp, "Socket path '%s' not found after %d retries",
>> +                                            path, CPR_MAX_RETRIES);
>> +    return false;
>> +}
>> +
>>   QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp)
>>   {
>>       MigrationAddress *addr = channel->addr;
>> @@ -28,6 +55,14 @@ QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp)
>>           QIOChannel *ioc = QIO_CHANNEL(sioc);
>>           SocketAddress *saddr = &addr->u.socket;
>>   
>> +        /*
>> +         * Verify that the cpr.sock Unix domain socket file exists and is ready
>> +         * before proceeding with the connection.
>> +         */
>> +        if (!cpr_validate_socket_path(addr->u.socket.u.q_unix.path, errp)) {
>> +            return NULL;
>> +        }
>> +
>>           if (qio_channel_socket_connect_sync(sioc, saddr, errp) < 0) {
>>               return NULL;
>>           }
>> -- 
>> 2.49.0
>>
>>
> 
> With regards,
> Daniel

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[JAEHOON KIM]

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


On 6/6/2025 8:40 AM, Fabiano Rosas wrote:
> Jaehoon Kim<jhkim@linux.ibm.com> writes:
>
>> When the source VM attempts to connect to the destination VM's Unix
>> domain socket(cpr.sock) during CPR transfer, the socket file might not
>> yet be exist if the destination side hasn't completed the bind
>> operation. This can lead to connection failures when running tests with
>> the qtest framework.
>>
> Could you provide us the output of qtest in this case? Are you simply running
> make check or something else?

Yes, I'm simply running 'make check-qtest-s390x'.

Here's the qtest output from the failure:
  # {
  #     "error": {
  #         "class": "GenericError",
  #         "desc": "Failed to connect to '/tmp/migration-test-ZC7Z72/cpr.sock': No such file or directory"
  #     }
  # }
  not ok /s390x/migration/mode/transfer - ERROR:../tests/qtest/libqtest.c:1453:qtest_vqmp_assert_success_ref: assertion failed: (qdict_haskey(response, "return"))
  Bail out!

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

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[JAEHOON KIM]

On 6/6/2025 9:14 AM, Steven Sistare wrote:
> On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
>> On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
>>> When the source VM attempts to connect to the destination VM's Unix
>>> domain socket(cpr.sock) during CPR transfer, the socket file might not
>>> yet be exist if the destination side hasn't completed the bind
>>> operation. This can lead to connection failures when running tests with
>>> the qtest framework.
>>
>> This sounds like a flawed test impl to me - whatever is initiating
>> the cpr operation on the source has done so prematurely - it should
>> ensure the dest is ready before starting the operation.
>>
>>> To address this, add cpr_validate_socket_path(), which wait for the
>>> socket file to appear. This avoids intermittent qtest failures 
>>> caused by
>>> early connection attempts.
>>
>> IMHO it is dubious to special case cpr in this way.
>
> I agree with Daniel, and unfortunately it is not just a test issue;
> every management framework that supports cpr-transfer must add logic to
> wait for the cpr socket to appear in the target before proceeding.
>
> This is analogous to waiting for the monitor socket to appear before
> connecting to it.
>
> - Steve

Thank you very much for your valuable review and feedback.

Just to clarify, the added cpr_validate_socket_path() function is not 
limited to the test framework.
It is part of the actual CPR implementation and is intended to ensure 
correct behavior in all cases, including outside of tests.

I mentioned the qtest failure simply as an example where this issue 
became apparent.

-Jaehoon Kim
>>> Signed-off-by: Jaehoon Kim <jhkim@linux.ibm.com>
>>> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
>>> ---
>>>   include/migration/cpr.h  |  1 +
>>>   migration/cpr-transfer.c | 35 +++++++++++++++++++++++++++++++++++
>>>   2 files changed, 36 insertions(+)
>>>
>>> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
>>> index 7561fc75ad..cc9384b4f9 100644
>>> --- a/include/migration/cpr.h
>>> +++ b/include/migration/cpr.h
>>> @@ -23,6 +23,7 @@ MigMode cpr_get_incoming_mode(void);
>>>   void cpr_set_incoming_mode(MigMode mode);
>>>   bool cpr_is_incoming(void);
>>>   +bool cpr_validate_socket_path(const char *path, Error **errp);
>>>   int cpr_state_save(MigrationChannel *channel, Error **errp);
>>>   int cpr_state_load(MigrationChannel *channel, Error **errp);
>>>   void cpr_state_close(void);
>>> diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
>>> index e1f140359c..3088ed323f 100644
>>> --- a/migration/cpr-transfer.c
>>> +++ b/migration/cpr-transfer.c
>>> @@ -17,6 +17,33 @@
>>>   #include "migration/vmstate.h"
>>>   #include "trace.h"
>>>   +#define CPR_MAX_RETRIES     50     /* Retry for up to 5 seconds */
>>> +#define CPR_RETRY_DELAY_US  100000 /* 100 ms per retry */
>>> +
>>> +bool cpr_validate_socket_path(const char *path, Error **errp)
>>> +{
>>> +    struct stat st;
>>> +    int retries = CPR_MAX_RETRIES;
>>> +
>>> +    do {
>>> +        if (!stat(path, &st) && S_ISSOCK(st.st_mode)) {
>>> +            return true;
>>> +        }
>>> +
>>> +        if (errno == ENOENT) {
>>> +            usleep(CPR_RETRY_DELAY_US);
>>> +        } else {
>>> +            error_setg_errno(errp, errno,
>>> +                "Unable to check status of socket path '%s'", path);
>>> +            return false;
>>> +        }
>>> +    } while (--retries > 0);
>>> +
>>> +    error_setg(errp, "Socket path '%s' not found after %d retries",
>>> +                                            path, CPR_MAX_RETRIES);
>>> +    return false;
>>> +}
>>> +
>>>   QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error 
>>> **errp)
>>>   {
>>>       MigrationAddress *addr = channel->addr;
>>> @@ -28,6 +55,14 @@ QEMUFile *cpr_transfer_output(MigrationChannel 
>>> *channel, Error **errp)
>>>           QIOChannel *ioc = QIO_CHANNEL(sioc);
>>>           SocketAddress *saddr = &addr->u.socket;
>>>   +        /*
>>> +         * Verify that the cpr.sock Unix domain socket file exists 
>>> and is ready
>>> +         * before proceeding with the connection.
>>> +         */
>>> +        if (!cpr_validate_socket_path(addr->u.socket.u.q_unix.path, 
>>> errp)) {
>>> +            return NULL;
>>> +        }
>>> +
>>>           if (qio_channel_socket_connect_sync(sioc, saddr, errp) < 0) {
>>>               return NULL;
>>>           }
>>> -- 
>>> 2.49.0
>>>
>>>
>>
>> With regards,
>> Daniel
>
>

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Steven Sistare]

On 6/6/2025 11:06 AM, JAEHOON KIM wrote:
> On 6/6/2025 9:14 AM, Steven Sistare wrote:
>> On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
>>> On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
>>>> When the source VM attempts to connect to the destination VM's Unix
>>>> domain socket(cpr.sock) during CPR transfer, the socket file might not
>>>> yet be exist if the destination side hasn't completed the bind
>>>> operation. This can lead to connection failures when running tests with
>>>> the qtest framework.
>>>
>>> This sounds like a flawed test impl to me - whatever is initiating
>>> the cpr operation on the source has done so prematurely - it should
>>> ensure the dest is ready before starting the operation.
>>>
>>>> To address this, add cpr_validate_socket_path(), which wait for the
>>>> socket file to appear. This avoids intermittent qtest failures caused by
>>>> early connection attempts.
>>>
>>> IMHO it is dubious to special case cpr in this way.
>>
>> I agree with Daniel, and unfortunately it is not just a test issue;
>> every management framework that supports cpr-transfer must add logic to
>> wait for the cpr socket to appear in the target before proceeding.
>>
>> This is analogous to waiting for the monitor socket to appear before
>> connecting to it.
>>
>> - Steve
> 
> Thank you very much for your valuable review and feedback.
> 
> Just to clarify, the added cpr_validate_socket_path() function is not limited to the test framework.
> It is part of the actual CPR implementation and is intended to ensure correct behavior in all cases, including outside of tests.
> 
> I mentioned the qtest failure simply as an example where this issue became apparent.

Yes, I understand that you understand :)
Are you willing to move your fix to the qtest?

- Steve

>>>> Signed-off-by: Jaehoon Kim <jhkim@linux.ibm.com>
>>>> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
>>>> ---
>>>>   include/migration/cpr.h  |  1 +
>>>>   migration/cpr-transfer.c | 35 +++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 36 insertions(+)
>>>>
>>>> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
>>>> index 7561fc75ad..cc9384b4f9 100644
>>>> --- a/include/migration/cpr.h
>>>> +++ b/include/migration/cpr.h
>>>> @@ -23,6 +23,7 @@ MigMode cpr_get_incoming_mode(void);
>>>>   void cpr_set_incoming_mode(MigMode mode);
>>>>   bool cpr_is_incoming(void);
>>>>   +bool cpr_validate_socket_path(const char *path, Error **errp);
>>>>   int cpr_state_save(MigrationChannel *channel, Error **errp);
>>>>   int cpr_state_load(MigrationChannel *channel, Error **errp);
>>>>   void cpr_state_close(void);
>>>> diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
>>>> index e1f140359c..3088ed323f 100644
>>>> --- a/migration/cpr-transfer.c
>>>> +++ b/migration/cpr-transfer.c
>>>> @@ -17,6 +17,33 @@
>>>>   #include "migration/vmstate.h"
>>>>   #include "trace.h"
>>>>   +#define CPR_MAX_RETRIES     50     /* Retry for up to 5 seconds */
>>>> +#define CPR_RETRY_DELAY_US  100000 /* 100 ms per retry */
>>>> +
>>>> +bool cpr_validate_socket_path(const char *path, Error **errp)
>>>> +{
>>>> +    struct stat st;
>>>> +    int retries = CPR_MAX_RETRIES;
>>>> +
>>>> +    do {
>>>> +        if (!stat(path, &st) && S_ISSOCK(st.st_mode)) {
>>>> +            return true;
>>>> +        }
>>>> +
>>>> +        if (errno == ENOENT) {
>>>> +            usleep(CPR_RETRY_DELAY_US);
>>>> +        } else {
>>>> +            error_setg_errno(errp, errno,
>>>> +                "Unable to check status of socket path '%s'", path);
>>>> +            return false;
>>>> +        }
>>>> +    } while (--retries > 0);
>>>> +
>>>> +    error_setg(errp, "Socket path '%s' not found after %d retries",
>>>> +                                            path, CPR_MAX_RETRIES);
>>>> +    return false;
>>>> +}
>>>> +
>>>>   QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp)
>>>>   {
>>>>       MigrationAddress *addr = channel->addr;
>>>> @@ -28,6 +55,14 @@ QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error **errp)
>>>>           QIOChannel *ioc = QIO_CHANNEL(sioc);
>>>>           SocketAddress *saddr = &addr->u.socket;
>>>>   +        /*
>>>> +         * Verify that the cpr.sock Unix domain socket file exists and is ready
>>>> +         * before proceeding with the connection.
>>>> +         */
>>>> +        if (!cpr_validate_socket_path(addr->u.socket.u.q_unix.path, errp)) {
>>>> +            return NULL;
>>>> +        }
>>>> +
>>>>           if (qio_channel_socket_connect_sync(sioc, saddr, errp) < 0) {
>>>>               return NULL;
>>>>           }
>>>> -- 
>>>> 2.49.0
>>>>
>>>>
>>>
>>> With regards,
>>> Daniel
>>
>>

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[JAEHOON KIM]

On 6/6/2025 10:12 AM, Steven Sistare wrote:
> On 6/6/2025 11:06 AM, JAEHOON KIM wrote:
>> On 6/6/2025 9:14 AM, Steven Sistare wrote:
>>> On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
>>>> On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
>>>>> When the source VM attempts to connect to the destination VM's Unix
>>>>> domain socket(cpr.sock) during CPR transfer, the socket file might 
>>>>> not
>>>>> yet be exist if the destination side hasn't completed the bind
>>>>> operation. This can lead to connection failures when running tests 
>>>>> with
>>>>> the qtest framework.
>>>>
>>>> This sounds like a flawed test impl to me - whatever is initiating
>>>> the cpr operation on the source has done so prematurely - it should
>>>> ensure the dest is ready before starting the operation.
>>>>
>>>>> To address this, add cpr_validate_socket_path(), which wait for the
>>>>> socket file to appear. This avoids intermittent qtest failures 
>>>>> caused by
>>>>> early connection attempts.
>>>>
>>>> IMHO it is dubious to special case cpr in this way.
>>>
>>> I agree with Daniel, and unfortunately it is not just a test issue;
>>> every management framework that supports cpr-transfer must add logic to
>>> wait for the cpr socket to appear in the target before proceeding.
>>>
>>> This is analogous to waiting for the monitor socket to appear before
>>> connecting to it.
>>>
>>> - Steve
>>
>> Thank you very much for your valuable review and feedback.
>>
>> Just to clarify, the added cpr_validate_socket_path() function is not 
>> limited to the test framework.
>> It is part of the actual CPR implementation and is intended to ensure 
>> correct behavior in all cases, including outside of tests.
>>
>> I mentioned the qtest failure simply as an example where this issue 
>> became apparent.
>
> Yes, I understand that you understand :)
> Are you willing to move your fix to the qtest?
>
> - Steve

Thank you for your question and feedback.

I agree that the issue could be addressed within the qtest framework to 
improve stability.

However, this socket readiness check is a fundamental part of CPR 
transfer process,
and I believe that incorporating cpr_validate_socket_path() directly 
into the CPR implementation helps ensure more reliable behavior
across all environments - not only during testing.

Just from my perspective, adding this logic to the CPR code does not 
introduce significant overhead or side effects.
I would appreciate if you could share more details about your concerns, 
so I can better address them.

- Jaehoon Kim

>
>>>>> Signed-off-by: Jaehoon Kim <jhkim@linux.ibm.com>
>>>>> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
>>>>> ---
>>>>>   include/migration/cpr.h  |  1 +
>>>>>   migration/cpr-transfer.c | 35 +++++++++++++++++++++++++++++++++++
>>>>>   2 files changed, 36 insertions(+)
>>>>>
>>>>> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
>>>>> index 7561fc75ad..cc9384b4f9 100644
>>>>> --- a/include/migration/cpr.h
>>>>> +++ b/include/migration/cpr.h
>>>>> @@ -23,6 +23,7 @@ MigMode cpr_get_incoming_mode(void);
>>>>>   void cpr_set_incoming_mode(MigMode mode);
>>>>>   bool cpr_is_incoming(void);
>>>>>   +bool cpr_validate_socket_path(const char *path, Error **errp);
>>>>>   int cpr_state_save(MigrationChannel *channel, Error **errp);
>>>>>   int cpr_state_load(MigrationChannel *channel, Error **errp);
>>>>>   void cpr_state_close(void);
>>>>> diff --git a/migration/cpr-transfer.c b/migration/cpr-transfer.c
>>>>> index e1f140359c..3088ed323f 100644
>>>>> --- a/migration/cpr-transfer.c
>>>>> +++ b/migration/cpr-transfer.c
>>>>> @@ -17,6 +17,33 @@
>>>>>   #include "migration/vmstate.h"
>>>>>   #include "trace.h"
>>>>>   +#define CPR_MAX_RETRIES     50     /* Retry for up to 5 seconds */
>>>>> +#define CPR_RETRY_DELAY_US  100000 /* 100 ms per retry */
>>>>> +
>>>>> +bool cpr_validate_socket_path(const char *path, Error **errp)
>>>>> +{
>>>>> +    struct stat st;
>>>>> +    int retries = CPR_MAX_RETRIES;
>>>>> +
>>>>> +    do {
>>>>> +        if (!stat(path, &st) && S_ISSOCK(st.st_mode)) {
>>>>> +            return true;
>>>>> +        }
>>>>> +
>>>>> +        if (errno == ENOENT) {
>>>>> +            usleep(CPR_RETRY_DELAY_US);
>>>>> +        } else {
>>>>> +            error_setg_errno(errp, errno,
>>>>> +                "Unable to check status of socket path '%s'", path);
>>>>> +            return false;
>>>>> +        }
>>>>> +    } while (--retries > 0);
>>>>> +
>>>>> +    error_setg(errp, "Socket path '%s' not found after %d retries",
>>>>> +                                            path, CPR_MAX_RETRIES);
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>>   QEMUFile *cpr_transfer_output(MigrationChannel *channel, Error 
>>>>> **errp)
>>>>>   {
>>>>>       MigrationAddress *addr = channel->addr;
>>>>> @@ -28,6 +55,14 @@ QEMUFile *cpr_transfer_output(MigrationChannel 
>>>>> *channel, Error **errp)
>>>>>           QIOChannel *ioc = QIO_CHANNEL(sioc);
>>>>>           SocketAddress *saddr = &addr->u.socket;
>>>>>   +        /*
>>>>> +         * Verify that the cpr.sock Unix domain socket file 
>>>>> exists and is ready
>>>>> +         * before proceeding with the connection.
>>>>> +         */
>>>>> +        if 
>>>>> (!cpr_validate_socket_path(addr->u.socket.u.q_unix.path, errp)) {
>>>>> +            return NULL;
>>>>> +        }
>>>>> +
>>>>>           if (qio_channel_socket_connect_sync(sioc, saddr, errp) < 
>>>>> 0) {
>>>>>               return NULL;
>>>>>           }
>>>>> -- 
>>>>> 2.49.0
>>>>>
>>>>>
>>>>
>>>> With regards,
>>>> Daniel
>>>
>>>
>

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Daniel P. Berrangé]

On Fri, Jun 06, 2025 at 10:37:28AM -0500, JAEHOON KIM wrote:
> 
> On 6/6/2025 10:12 AM, Steven Sistare wrote:
> > On 6/6/2025 11:06 AM, JAEHOON KIM wrote:
> > > On 6/6/2025 9:14 AM, Steven Sistare wrote:
> > > > On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
> > > > > On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
> > > > > > When the source VM attempts to connect to the destination VM's Unix
> > > > > > domain socket(cpr.sock) during CPR transfer, the socket
> > > > > > file might not
> > > > > > yet be exist if the destination side hasn't completed the bind
> > > > > > operation. This can lead to connection failures when
> > > > > > running tests with
> > > > > > the qtest framework.
> > > > > 
> > > > > This sounds like a flawed test impl to me - whatever is initiating
> > > > > the cpr operation on the source has done so prematurely - it should
> > > > > ensure the dest is ready before starting the operation.
> > > > > 
> > > > > > To address this, add cpr_validate_socket_path(), which wait for the
> > > > > > socket file to appear. This avoids intermittent qtest
> > > > > > failures caused by
> > > > > > early connection attempts.
> > > > > 
> > > > > IMHO it is dubious to special case cpr in this way.
> > > > 
> > > > I agree with Daniel, and unfortunately it is not just a test issue;
> > > > every management framework that supports cpr-transfer must add logic to
> > > > wait for the cpr socket to appear in the target before proceeding.
> > > > 
> > > > This is analogous to waiting for the monitor socket to appear before
> > > > connecting to it.
> > > > 
> > > > - Steve
> > > 
> > > Thank you very much for your valuable review and feedback.
> > > 
> > > Just to clarify, the added cpr_validate_socket_path() function is
> > > not limited to the test framework.
> > > It is part of the actual CPR implementation and is intended to
> > > ensure correct behavior in all cases, including outside of tests.
> > > 
> > > I mentioned the qtest failure simply as an example where this issue
> > > became apparent.
> > 
> > Yes, I understand that you understand :)
> > Are you willing to move your fix to the qtest?
> > 
> > - Steve
> 
> Thank you for your question and feedback.
> 
> I agree that the issue could be addressed within the qtest framework to
> improve stability.
> 
> However, this socket readiness check is a fundamental part of CPR transfer
> process,
> and I believe that incorporating cpr_validate_socket_path() directly into
> the CPR implementation helps ensure more reliable behavior
> across all environments - not only during testing.
> 
> Just from my perspective, adding this logic to the CPR code does not
> introduce significant overhead or side effects.
> I would appreciate if you could share more details about your concerns, so I
> can better address them.

Requiring a busy wait like this is a sign of a design problem.

There needs to be a way to setup the incoming socket listener
without resorting to busy waiting - that's showing a lack of
synchronization.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Daniel P. Berrangé]

On Fri, Jun 06, 2025 at 09:48:41AM -0500, JAEHOON KIM wrote:
> 
> On 6/6/2025 8:40 AM, Fabiano Rosas wrote:
> > Jaehoon Kim<jhkim@linux.ibm.com> writes:
> > 
> > > When the source VM attempts to connect to the destination VM's Unix
> > > domain socket(cpr.sock) during CPR transfer, the socket file might not
> > > yet be exist if the destination side hasn't completed the bind
> > > operation. This can lead to connection failures when running tests with
> > > the qtest framework.
> > > 
> > Could you provide us the output of qtest in this case? Are you simply running
> > make check or something else?
> 
> Yes, I'm simply running 'make check-qtest-s390x'.
> 
> Here's the qtest output from the failure:
>  # {
>  #     "error": {
>  #         "class": "GenericError",
>  #         "desc": "Failed to connect to '/tmp/migration-test-ZC7Z72/cpr.sock': No such file or directory"
>  #     }
>  # }
>  not ok /s390x/migration/mode/transfer - ERROR:../tests/qtest/libqtest.c:1453:qtest_vqmp_assert_success_ref: assertion failed: (qdict_haskey(response, "return"))
>  Bail out!

So this is showing a failure when using

  $QEMU -incoming cpr:...address...

as opposed to

  $QEMU -incoming cpr:defer

I presume in the former case, the test is spawning QEMU, but the startup
of QEMU & its listening on the UNIX socket is not synchronized with the
parent process.

In the latter case usnig 'defer', listening will be synchronized by the
QMP command used to setup the incoming socket.


So why do we see a race with "-incoming cpr:..address", but not
with a traditional "-incoming ...address.." for non-CPR code ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Steven Sistare]

On 6/6/2025 11:43 AM, Daniel P. Berrangé wrote:
> On Fri, Jun 06, 2025 at 10:37:28AM -0500, JAEHOON KIM wrote:
>> On 6/6/2025 10:12 AM, Steven Sistare wrote:
>>> On 6/6/2025 11:06 AM, JAEHOON KIM wrote:
>>>> On 6/6/2025 9:14 AM, Steven Sistare wrote:
>>>>> On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
>>>>>> On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
>>>>>>> When the source VM attempts to connect to the destination VM's Unix
>>>>>>> domain socket(cpr.sock) during CPR transfer, the socket
>>>>>>> file might not
>>>>>>> yet be exist if the destination side hasn't completed the bind
>>>>>>> operation. This can lead to connection failures when
>>>>>>> running tests with
>>>>>>> the qtest framework.
>>>>>>
>>>>>> This sounds like a flawed test impl to me - whatever is initiating
>>>>>> the cpr operation on the source has done so prematurely - it should
>>>>>> ensure the dest is ready before starting the operation.
>>>>>>
>>>>>>> To address this, add cpr_validate_socket_path(), which wait for the
>>>>>>> socket file to appear. This avoids intermittent qtest
>>>>>>> failures caused by
>>>>>>> early connection attempts.
>>>>>>
>>>>>> IMHO it is dubious to special case cpr in this way.
>>>>>
>>>>> I agree with Daniel, and unfortunately it is not just a test issue;
>>>>> every management framework that supports cpr-transfer must add logic to
>>>>> wait for the cpr socket to appear in the target before proceeding.
>>>>>
>>>>> This is analogous to waiting for the monitor socket to appear before
>>>>> connecting to it.
>>>>>
>>>>> - Steve
>>>>
>>>> Thank you very much for your valuable review and feedback.
>>>>
>>>> Just to clarify, the added cpr_validate_socket_path() function is
>>>> not limited to the test framework.
>>>> It is part of the actual CPR implementation and is intended to
>>>> ensure correct behavior in all cases, including outside of tests.
>>>>
>>>> I mentioned the qtest failure simply as an example where this issue
>>>> became apparent.
>>>
>>> Yes, I understand that you understand :)
>>> Are you willing to move your fix to the qtest?
>>>
>>> - Steve
>>
>> Thank you for your question and feedback.
>>
>> I agree that the issue could be addressed within the qtest framework to
>> improve stability.
>>
>> However, this socket readiness check is a fundamental part of CPR transfer
>> process,
>> and I believe that incorporating cpr_validate_socket_path() directly into
>> the CPR implementation helps ensure more reliable behavior
>> across all environments - not only during testing.
>>
>> Just from my perspective, adding this logic to the CPR code does not
>> introduce significant overhead or side effects.
>> I would appreciate if you could share more details about your concerns, so I
>> can better address them.
> 
> Requiring a busy wait like this is a sign of a design problem.
> 
> There needs to be a way to setup the incoming socket listener
> without resorting to busy waiting - that's showing a lack of
> synchronization.

How is this a design problem?  If I start a program that creates a listening unix
domain socket, I cannot attempt to connect to it until the socket is created and
listening. Clients face the same issue when starting qemu and connecting to the
monitor socket.

- Steve

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Daniel P. Berrangé]

On Fri, Jun 06, 2025 at 11:50:10AM -0400, Steven Sistare wrote:
> On 6/6/2025 11:43 AM, Daniel P. Berrangé wrote:
> > On Fri, Jun 06, 2025 at 10:37:28AM -0500, JAEHOON KIM wrote:
> > > On 6/6/2025 10:12 AM, Steven Sistare wrote:
> > > > On 6/6/2025 11:06 AM, JAEHOON KIM wrote:
> > > > > On 6/6/2025 9:14 AM, Steven Sistare wrote:
> > > > > > On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
> > > > > > > On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
> > > > > > > > When the source VM attempts to connect to the destination VM's Unix
> > > > > > > > domain socket(cpr.sock) during CPR transfer, the socket
> > > > > > > > file might not
> > > > > > > > yet be exist if the destination side hasn't completed the bind
> > > > > > > > operation. This can lead to connection failures when
> > > > > > > > running tests with
> > > > > > > > the qtest framework.
> > > > > > > 
> > > > > > > This sounds like a flawed test impl to me - whatever is initiating
> > > > > > > the cpr operation on the source has done so prematurely - it should
> > > > > > > ensure the dest is ready before starting the operation.
> > > > > > > 
> > > > > > > > To address this, add cpr_validate_socket_path(), which wait for the
> > > > > > > > socket file to appear. This avoids intermittent qtest
> > > > > > > > failures caused by
> > > > > > > > early connection attempts.
> > > > > > > 
> > > > > > > IMHO it is dubious to special case cpr in this way.
> > > > > > 
> > > > > > I agree with Daniel, and unfortunately it is not just a test issue;
> > > > > > every management framework that supports cpr-transfer must add logic to
> > > > > > wait for the cpr socket to appear in the target before proceeding.
> > > > > > 
> > > > > > This is analogous to waiting for the monitor socket to appear before
> > > > > > connecting to it.
> > > > > > 
> > > > > > - Steve
> > > > > 
> > > > > Thank you very much for your valuable review and feedback.
> > > > > 
> > > > > Just to clarify, the added cpr_validate_socket_path() function is
> > > > > not limited to the test framework.
> > > > > It is part of the actual CPR implementation and is intended to
> > > > > ensure correct behavior in all cases, including outside of tests.
> > > > > 
> > > > > I mentioned the qtest failure simply as an example where this issue
> > > > > became apparent.
> > > > 
> > > > Yes, I understand that you understand :)
> > > > Are you willing to move your fix to the qtest?
> > > > 
> > > > - Steve
> > > 
> > > Thank you for your question and feedback.
> > > 
> > > I agree that the issue could be addressed within the qtest framework to
> > > improve stability.
> > > 
> > > However, this socket readiness check is a fundamental part of CPR transfer
> > > process,
> > > and I believe that incorporating cpr_validate_socket_path() directly into
> > > the CPR implementation helps ensure more reliable behavior
> > > across all environments - not only during testing.
> > > 
> > > Just from my perspective, adding this logic to the CPR code does not
> > > introduce significant overhead or side effects.
> > > I would appreciate if you could share more details about your concerns, so I
> > > can better address them.
> > 
> > Requiring a busy wait like this is a sign of a design problem.
> > 
> > There needs to be a way to setup the incoming socket listener
> > without resorting to busy waiting - that's showing a lack of
> > synchronization.
> 
> How is this a design problem?  If I start a program that creates a listening unix
> domain socket, I cannot attempt to connect to it until the socket is created and
> listening. Clients face the same issue when starting qemu and connecting to the
> monitor socket.

Yes, the monitor has the same conceptual problem, and this caused problems
for libvirt starting QEMU for many years.

With the busy wait you risk looping forever if the child (target) QEMU
already exited for some reason without ever creating the socket. You
can mitigate this by using 'kill($PID, 0)' in the loop and looking
for -ERSCH, but this only works if you know the pid involved.


One option is to use 'daemonize' such that when the parent sees the initial
QEMU process leader exit, the parent has a guarantee that the daemonized
QEMU already has the UNIX listener open, and any failure indicates QEMU
already quit.


The other option is to use FD passing such that QEMU is not responsible
for opening the listener socket - it gets passed a pre-opened listener
FD, so the parent has a guarantee it can successfull connect immediately
and any failure indicates QEMU already quit.


For the tests, passing a pre-opened UNIX socket FD could work, but I'm
still curious why this is only a problem for the CPR tests, and not
the other migration tests which don't use 'defer'. What has made CPR
special to expose a race ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Steven Sistare]

On 6/6/2025 12:06 PM, Daniel P. Berrangé wrote:
> On Fri, Jun 06, 2025 at 11:50:10AM -0400, Steven Sistare wrote:
>> On 6/6/2025 11:43 AM, Daniel P. Berrangé wrote:
>>> On Fri, Jun 06, 2025 at 10:37:28AM -0500, JAEHOON KIM wrote:
>>>> On 6/6/2025 10:12 AM, Steven Sistare wrote:
>>>>> On 6/6/2025 11:06 AM, JAEHOON KIM wrote:
>>>>>> On 6/6/2025 9:14 AM, Steven Sistare wrote:
>>>>>>> On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
>>>>>>>> On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
>>>>>>>>> When the source VM attempts to connect to the destination VM's Unix
>>>>>>>>> domain socket(cpr.sock) during CPR transfer, the socket
>>>>>>>>> file might not
>>>>>>>>> yet be exist if the destination side hasn't completed the bind
>>>>>>>>> operation. This can lead to connection failures when
>>>>>>>>> running tests with
>>>>>>>>> the qtest framework.
>>>>>>>>
>>>>>>>> This sounds like a flawed test impl to me - whatever is initiating
>>>>>>>> the cpr operation on the source has done so prematurely - it should
>>>>>>>> ensure the dest is ready before starting the operation.
>>>>>>>>
>>>>>>>>> To address this, add cpr_validate_socket_path(), which wait for the
>>>>>>>>> socket file to appear. This avoids intermittent qtest
>>>>>>>>> failures caused by
>>>>>>>>> early connection attempts.
>>>>>>>>
>>>>>>>> IMHO it is dubious to special case cpr in this way.
>>>>>>>
>>>>>>> I agree with Daniel, and unfortunately it is not just a test issue;
>>>>>>> every management framework that supports cpr-transfer must add logic to
>>>>>>> wait for the cpr socket to appear in the target before proceeding.
>>>>>>>
>>>>>>> This is analogous to waiting for the monitor socket to appear before
>>>>>>> connecting to it.
>>>>>>>
>>>>>>> - Steve
>>>>>>
>>>>>> Thank you very much for your valuable review and feedback.
>>>>>>
>>>>>> Just to clarify, the added cpr_validate_socket_path() function is
>>>>>> not limited to the test framework.
>>>>>> It is part of the actual CPR implementation and is intended to
>>>>>> ensure correct behavior in all cases, including outside of tests.
>>>>>>
>>>>>> I mentioned the qtest failure simply as an example where this issue
>>>>>> became apparent.
>>>>>
>>>>> Yes, I understand that you understand :)
>>>>> Are you willing to move your fix to the qtest?
>>>>>
>>>>> - Steve
>>>>
>>>> Thank you for your question and feedback.
>>>>
>>>> I agree that the issue could be addressed within the qtest framework to
>>>> improve stability.
>>>>
>>>> However, this socket readiness check is a fundamental part of CPR transfer
>>>> process,
>>>> and I believe that incorporating cpr_validate_socket_path() directly into
>>>> the CPR implementation helps ensure more reliable behavior
>>>> across all environments - not only during testing.
>>>>
>>>> Just from my perspective, adding this logic to the CPR code does not
>>>> introduce significant overhead or side effects.
>>>> I would appreciate if you could share more details about your concerns, so I
>>>> can better address them.
>>>
>>> Requiring a busy wait like this is a sign of a design problem.
>>>
>>> There needs to be a way to setup the incoming socket listener
>>> without resorting to busy waiting - that's showing a lack of
>>> synchronization.
>>
>> How is this a design problem?  If I start a program that creates a listening unix
>> domain socket, I cannot attempt to connect to it until the socket is created and
>> listening. Clients face the same issue when starting qemu and connecting to the
>> monitor socket.
> 
> Yes, the monitor has the same conceptual problem, and this caused problems
> for libvirt starting QEMU for many years.
> 
> With the busy wait you risk looping forever if the child (target) QEMU
> already exited for some reason without ever creating the socket. You
> can mitigate this by using 'kill($PID, 0)' in the loop and looking
> for -ERSCH, but this only works if you know the pid involved.
> 
> One option is to use 'daemonize' such that when the parent sees the initial
> QEMU process leader exit, the parent has a guarantee that the daemonized
> QEMU already has the UNIX listener open, and any failure indicates QEMU
> already quit.
> 
> The other option is to use FD passing such that QEMU is not responsible
> for opening the listener socket - it gets passed a pre-opened listener
> FD, so the parent has a guarantee it can successfull connect immediately
> and any failure indicates QEMU already quit.
> 
> For the tests, passing a pre-opened UNIX socket FD could work, but I'm
> still curious why this is only a problem for the CPR tests, and not
> the other migration tests which don't use 'defer'. What has made CPR
> special to expose a race ?

For normal migration, target qemu listens on the migration socket, then listens
on the monitor.  After the client connects to the monitor (waiting for it to appear
as needed), them the migration socket already exists.

For cpr, target qemu creates the cpr socket and listens before the monitor is
started, which is necessary because cpr state is needed before backends or
devices are created.

A few months back I sent a series to start the monitor first (I think I called
it the precreate phase), but it was derailed over discussions about allowing
qemu to start with no arguments and be configured exclusively via the monitor.

- Steve

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[JAEHOON KIM]

On 6/6/2025 12:04 PM, Steven Sistare wrote:
> On 6/6/2025 12:06 PM, Daniel P. Berrangé wrote:
>> On Fri, Jun 06, 2025 at 11:50:10AM -0400, Steven Sistare wrote:
>>> On 6/6/2025 11:43 AM, Daniel P. Berrangé wrote:
>>>> On Fri, Jun 06, 2025 at 10:37:28AM -0500, JAEHOON KIM wrote:
>>>>> On 6/6/2025 10:12 AM, Steven Sistare wrote:
>>>>>> On 6/6/2025 11:06 AM, JAEHOON KIM wrote:
>>>>>>> On 6/6/2025 9:14 AM, Steven Sistare wrote:
>>>>>>>> On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
>>>>>>>>> On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
>>>>>>>>>> When the source VM attempts to connect to the destination 
>>>>>>>>>> VM's Unix
>>>>>>>>>> domain socket(cpr.sock) during CPR transfer, the socket
>>>>>>>>>> file might not
>>>>>>>>>> yet be exist if the destination side hasn't completed the bind
>>>>>>>>>> operation. This can lead to connection failures when
>>>>>>>>>> running tests with
>>>>>>>>>> the qtest framework.
>>>>>>>>>
>>>>>>>>> This sounds like a flawed test impl to me - whatever is 
>>>>>>>>> initiating
>>>>>>>>> the cpr operation on the source has done so prematurely - it 
>>>>>>>>> should
>>>>>>>>> ensure the dest is ready before starting the operation.
>>>>>>>>>
>>>>>>>>>> To address this, add cpr_validate_socket_path(), which wait 
>>>>>>>>>> for the
>>>>>>>>>> socket file to appear. This avoids intermittent qtest
>>>>>>>>>> failures caused by
>>>>>>>>>> early connection attempts.
>>>>>>>>>
>>>>>>>>> IMHO it is dubious to special case cpr in this way.
>>>>>>>>
>>>>>>>> I agree with Daniel, and unfortunately it is not just a test 
>>>>>>>> issue;
>>>>>>>> every management framework that supports cpr-transfer must add 
>>>>>>>> logic to
>>>>>>>> wait for the cpr socket to appear in the target before proceeding.
>>>>>>>>
>>>>>>>> This is analogous to waiting for the monitor socket to appear 
>>>>>>>> before
>>>>>>>> connecting to it.
>>>>>>>>
>>>>>>>> - Steve
>>>>>>>
>>>>>>> Thank you very much for your valuable review and feedback.
>>>>>>>
>>>>>>> Just to clarify, the added cpr_validate_socket_path() function is
>>>>>>> not limited to the test framework.
>>>>>>> It is part of the actual CPR implementation and is intended to
>>>>>>> ensure correct behavior in all cases, including outside of tests.
>>>>>>>
>>>>>>> I mentioned the qtest failure simply as an example where this issue
>>>>>>> became apparent.
>>>>>>
>>>>>> Yes, I understand that you understand :)
>>>>>> Are you willing to move your fix to the qtest?
>>>>>>
>>>>>> - Steve
>>>>>
>>>>> Thank you for your question and feedback.
>>>>>
>>>>> I agree that the issue could be addressed within the qtest 
>>>>> framework to
>>>>> improve stability.
>>>>>
>>>>> However, this socket readiness check is a fundamental part of CPR 
>>>>> transfer
>>>>> process,
>>>>> and I believe that incorporating cpr_validate_socket_path() 
>>>>> directly into
>>>>> the CPR implementation helps ensure more reliable behavior
>>>>> across all environments - not only during testing.
>>>>>
>>>>> Just from my perspective, adding this logic to the CPR code does not
>>>>> introduce significant overhead or side effects.
>>>>> I would appreciate if you could share more details about your 
>>>>> concerns, so I
>>>>> can better address them.
>>>>
>>>> Requiring a busy wait like this is a sign of a design problem.
>>>>
>>>> There needs to be a way to setup the incoming socket listener
>>>> without resorting to busy waiting - that's showing a lack of
>>>> synchronization.
>>>
>>> How is this a design problem?  If I start a program that creates a 
>>> listening unix
>>> domain socket, I cannot attempt to connect to it until the socket is 
>>> created and
>>> listening. Clients face the same issue when starting qemu and 
>>> connecting to the
>>> monitor socket.
>>
>> Yes, the monitor has the same conceptual problem, and this caused 
>> problems
>> for libvirt starting QEMU for many years.
>>
>> With the busy wait you risk looping forever if the child (target) QEMU
>> already exited for some reason without ever creating the socket. You
>> can mitigate this by using 'kill($PID, 0)' in the loop and looking
>> for -ERSCH, but this only works if you know the pid involved.
>>
>> One option is to use 'daemonize' such that when the parent sees the 
>> initial
>> QEMU process leader exit, the parent has a guarantee that the daemonized
>> QEMU already has the UNIX listener open, and any failure indicates QEMU
>> already quit.
>>
>> The other option is to use FD passing such that QEMU is not responsible
>> for opening the listener socket - it gets passed a pre-opened listener
>> FD, so the parent has a guarantee it can successfull connect immediately
>> and any failure indicates QEMU already quit.
>>
>> For the tests, passing a pre-opened UNIX socket FD could work, but I'm
>> still curious why this is only a problem for the CPR tests, and not
>> the other migration tests which don't use 'defer'. What has made CPR
>> special to expose a race ?
>
> For normal migration, target qemu listens on the migration socket, 
> then listens
> on the monitor.  After the client connects to the monitor (waiting for 
> it to appear
> as needed), them the migration socket already exists.
>
> For cpr, target qemu creates the cpr socket and listens before the 
> monitor is
> started, which is necessary because cpr state is needed before 
> backends or
> devices are created.
>
> A few months back I sent a series to start the monitor first (I think 
> I called
> it the precreate phase), but it was derailed over discussions about 
> allowing
> qemu to start with no arguments and be configured exclusively via the 
> monitor.
>
> - Steve

Thank you for sharing your thoughts.

I agree that busy waiting is not ideal.
However, considering the timing of when target QEMU creates and begins 
listening on the socket,
I think there is currently no reliable way for the host to check the 
socket's listening state.
This also implies that FD passing is not a viable option in this case.

As for the 'defer' option in qtest,
it doesn't cause race-condition issues because the target enters the 
listening state during the option processing.

Of course, to address this issue,
I could create a wait_for_socket() function similar to wait_for_serial() 
in qtest framework.
Since the socket might already be created, I cannot simply wait for the 
file to appear using file system notification APIs like inotify,
so busy-waiting would still be necessary.

I would appreciate hearing any further thoughts you might have on this.

- Jaehoon Kim.

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Steven Sistare]

On 6/6/2025 2:06 PM, JAEHOON KIM wrote:
> On 6/6/2025 12:04 PM, Steven Sistare wrote:
>> On 6/6/2025 12:06 PM, Daniel P. Berrangé wrote:
>>> On Fri, Jun 06, 2025 at 11:50:10AM -0400, Steven Sistare wrote:
>>>> On 6/6/2025 11:43 AM, Daniel P. Berrangé wrote:
>>>>> On Fri, Jun 06, 2025 at 10:37:28AM -0500, JAEHOON KIM wrote:
>>>>>> On 6/6/2025 10:12 AM, Steven Sistare wrote:
>>>>>>> On 6/6/2025 11:06 AM, JAEHOON KIM wrote:
>>>>>>>> On 6/6/2025 9:14 AM, Steven Sistare wrote:
>>>>>>>>> On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
>>>>>>>>>> On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
>>>>>>>>>>> When the source VM attempts to connect to the destination VM's Unix
>>>>>>>>>>> domain socket(cpr.sock) during CPR transfer, the socket
>>>>>>>>>>> file might not
>>>>>>>>>>> yet be exist if the destination side hasn't completed the bind
>>>>>>>>>>> operation. This can lead to connection failures when
>>>>>>>>>>> running tests with
>>>>>>>>>>> the qtest framework.
>>>>>>>>>>
>>>>>>>>>> This sounds like a flawed test impl to me - whatever is initiating
>>>>>>>>>> the cpr operation on the source has done so prematurely - it should
>>>>>>>>>> ensure the dest is ready before starting the operation.
>>>>>>>>>>
>>>>>>>>>>> To address this, add cpr_validate_socket_path(), which wait for the
>>>>>>>>>>> socket file to appear. This avoids intermittent qtest
>>>>>>>>>>> failures caused by
>>>>>>>>>>> early connection attempts.
>>>>>>>>>>
>>>>>>>>>> IMHO it is dubious to special case cpr in this way.
>>>>>>>>>
>>>>>>>>> I agree with Daniel, and unfortunately it is not just a test issue;
>>>>>>>>> every management framework that supports cpr-transfer must add logic to
>>>>>>>>> wait for the cpr socket to appear in the target before proceeding.
>>>>>>>>>
>>>>>>>>> This is analogous to waiting for the monitor socket to appear before
>>>>>>>>> connecting to it.
>>>>>>>>>
>>>>>>>>> - Steve
>>>>>>>>
>>>>>>>> Thank you very much for your valuable review and feedback.
>>>>>>>>
>>>>>>>> Just to clarify, the added cpr_validate_socket_path() function is
>>>>>>>> not limited to the test framework.
>>>>>>>> It is part of the actual CPR implementation and is intended to
>>>>>>>> ensure correct behavior in all cases, including outside of tests.
>>>>>>>>
>>>>>>>> I mentioned the qtest failure simply as an example where this issue
>>>>>>>> became apparent.
>>>>>>>
>>>>>>> Yes, I understand that you understand :)
>>>>>>> Are you willing to move your fix to the qtest?
>>>>>>>
>>>>>>> - Steve
>>>>>>
>>>>>> Thank you for your question and feedback.
>>>>>>
>>>>>> I agree that the issue could be addressed within the qtest framework to
>>>>>> improve stability.
>>>>>>
>>>>>> However, this socket readiness check is a fundamental part of CPR transfer
>>>>>> process,
>>>>>> and I believe that incorporating cpr_validate_socket_path() directly into
>>>>>> the CPR implementation helps ensure more reliable behavior
>>>>>> across all environments - not only during testing.
>>>>>>
>>>>>> Just from my perspective, adding this logic to the CPR code does not
>>>>>> introduce significant overhead or side effects.
>>>>>> I would appreciate if you could share more details about your concerns, so I
>>>>>> can better address them.
>>>>>
>>>>> Requiring a busy wait like this is a sign of a design problem.
>>>>>
>>>>> There needs to be a way to setup the incoming socket listener
>>>>> without resorting to busy waiting - that's showing a lack of
>>>>> synchronization.
>>>>
>>>> How is this a design problem?  If I start a program that creates a listening unix
>>>> domain socket, I cannot attempt to connect to it until the socket is created and
>>>> listening. Clients face the same issue when starting qemu and connecting to the
>>>> monitor socket.
>>>
>>> Yes, the monitor has the same conceptual problem, and this caused problems
>>> for libvirt starting QEMU for many years.
>>>
>>> With the busy wait you risk looping forever if the child (target) QEMU
>>> already exited for some reason without ever creating the socket. You
>>> can mitigate this by using 'kill($PID, 0)' in the loop and looking
>>> for -ERSCH, but this only works if you know the pid involved.
>>>
>>> One option is to use 'daemonize' such that when the parent sees the initial
>>> QEMU process leader exit, the parent has a guarantee that the daemonized
>>> QEMU already has the UNIX listener open, and any failure indicates QEMU
>>> already quit.
>>>
>>> The other option is to use FD passing such that QEMU is not responsible
>>> for opening the listener socket - it gets passed a pre-opened listener
>>> FD, so the parent has a guarantee it can successfull connect immediately
>>> and any failure indicates QEMU already quit.
>>>
>>> For the tests, passing a pre-opened UNIX socket FD could work, but I'm
>>> still curious why this is only a problem for the CPR tests, and not
>>> the other migration tests which don't use 'defer'. What has made CPR
>>> special to expose a race ?
>>
>> For normal migration, target qemu listens on the migration socket, then listens
>> on the monitor.  After the client connects to the monitor (waiting for it to appear
>> as needed), them the migration socket already exists.
>>
>> For cpr, target qemu creates the cpr socket and listens before the monitor is
>> started, which is necessary because cpr state is needed before backends or
>> devices are created.
>>
>> A few months back I sent a series to start the monitor first (I think I called
>> it the precreate phase), but it was derailed over discussions about allowing
>> qemu to start with no arguments and be configured exclusively via the monitor.
>>
>> - Steve
> 
> Thank you for sharing your thoughts.
> 
> I agree that busy waiting is not ideal.
> However, considering the timing of when target QEMU creates and begins listening on the socket,
> I think there is currently no reliable way for the host to check the socket's listening state.
> This also implies that FD passing is not a viable option in this case.
> 
> As for the 'defer' option in qtest,
> it doesn't cause race-condition issues because the target enters the listening state during the option processing.
> 
> Of course, to address this issue,
> I could create a wait_for_socket() function similar to wait_for_serial() in qtest framework.
> Since the socket might already be created, I cannot simply wait for the file to appear using file system notification APIs like inotify,
> so busy-waiting would still be necessary.
> 
> I would appreciate hearing any further thoughts you might have on this.

The easiest solution, with no interface changes, is adding wait_for_socket() in qtest,
with this addition from Daniel:

   "With the busy wait you risk looping forever if the child (target) QEMU
   already exited for some reason without ever creating the socket. You
   can mitigate this by using 'kill($PID, 0)' in the loop and looking
   for -ERSCH, but this only works if you know the pid involved."

Daniel also suggested:
   "For the tests, passing a pre-opened UNIX socket FD could work"

Note we can not use any of the standard chardev options to specify such a socket,
because the cpr socket is created before chardevs are created.

Perhaps we could specify the fd in an extension of the MigrationChannel MigrationAddress.
{ 'union': 'MigrationAddress',
   'base': { 'transport' : 'MigrationAddressType'},
   'discriminator': 'transport',
   'data': {
     'socket': 'SocketAddress',
     'exec': 'MigrationExecCommand',
     'rdma': 'InetSocketAddress',
     'file': 'FileMigrationArgs',
     'fd':   'FdMigrationArgs' } }           <-- add this

That would be useful for all clients, but this is asking a lot from you,
when you are just trying to fix the tests.

- Steve

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[JAEHOON KIM]

On 6/6/2025 2:37 PM, Steven Sistare wrote:
> On 6/6/2025 2:06 PM, JAEHOON KIM wrote:
>> On 6/6/2025 12:04 PM, Steven Sistare wrote:
>>> On 6/6/2025 12:06 PM, Daniel P. Berrangé wrote:
>>>> On Fri, Jun 06, 2025 at 11:50:10AM -0400, Steven Sistare wrote:
>>>>> On 6/6/2025 11:43 AM, Daniel P. Berrangé wrote:
>>>>>> On Fri, Jun 06, 2025 at 10:37:28AM -0500, JAEHOON KIM wrote:
>>>>>>> On 6/6/2025 10:12 AM, Steven Sistare wrote:
>>>>>>>> On 6/6/2025 11:06 AM, JAEHOON KIM wrote:
>>>>>>>>> On 6/6/2025 9:14 AM, Steven Sistare wrote:
>>>>>>>>>> On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
>>>>>>>>>>> On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
>>>>>>>>>>>> When the source VM attempts to connect to the destination 
>>>>>>>>>>>> VM's Unix
>>>>>>>>>>>> domain socket(cpr.sock) during CPR transfer, the socket
>>>>>>>>>>>> file might not
>>>>>>>>>>>> yet be exist if the destination side hasn't completed the bind
>>>>>>>>>>>> operation. This can lead to connection failures when
>>>>>>>>>>>> running tests with
>>>>>>>>>>>> the qtest framework.
>>>>>>>>>>>
>>>>>>>>>>> This sounds like a flawed test impl to me - whatever is 
>>>>>>>>>>> initiating
>>>>>>>>>>> the cpr operation on the source has done so prematurely - it 
>>>>>>>>>>> should
>>>>>>>>>>> ensure the dest is ready before starting the operation.
>>>>>>>>>>>
>>>>>>>>>>>> To address this, add cpr_validate_socket_path(), which wait 
>>>>>>>>>>>> for the
>>>>>>>>>>>> socket file to appear. This avoids intermittent qtest
>>>>>>>>>>>> failures caused by
>>>>>>>>>>>> early connection attempts.
>>>>>>>>>>>
>>>>>>>>>>> IMHO it is dubious to special case cpr in this way.
>>>>>>>>>>
>>>>>>>>>> I agree with Daniel, and unfortunately it is not just a test 
>>>>>>>>>> issue;
>>>>>>>>>> every management framework that supports cpr-transfer must 
>>>>>>>>>> add logic to
>>>>>>>>>> wait for the cpr socket to appear in the target before 
>>>>>>>>>> proceeding.
>>>>>>>>>>
>>>>>>>>>> This is analogous to waiting for the monitor socket to appear 
>>>>>>>>>> before
>>>>>>>>>> connecting to it.
>>>>>>>>>>
>>>>>>>>>> - Steve
>>>>>>>>>
>>>>>>>>> Thank you very much for your valuable review and feedback.
>>>>>>>>>
>>>>>>>>> Just to clarify, the added cpr_validate_socket_path() function is
>>>>>>>>> not limited to the test framework.
>>>>>>>>> It is part of the actual CPR implementation and is intended to
>>>>>>>>> ensure correct behavior in all cases, including outside of tests.
>>>>>>>>>
>>>>>>>>> I mentioned the qtest failure simply as an example where this 
>>>>>>>>> issue
>>>>>>>>> became apparent.
>>>>>>>>
>>>>>>>> Yes, I understand that you understand :)
>>>>>>>> Are you willing to move your fix to the qtest?
>>>>>>>>
>>>>>>>> - Steve
>>>>>>>
>>>>>>> Thank you for your question and feedback.
>>>>>>>
>>>>>>> I agree that the issue could be addressed within the qtest 
>>>>>>> framework to
>>>>>>> improve stability.
>>>>>>>
>>>>>>> However, this socket readiness check is a fundamental part of 
>>>>>>> CPR transfer
>>>>>>> process,
>>>>>>> and I believe that incorporating cpr_validate_socket_path() 
>>>>>>> directly into
>>>>>>> the CPR implementation helps ensure more reliable behavior
>>>>>>> across all environments - not only during testing.
>>>>>>>
>>>>>>> Just from my perspective, adding this logic to the CPR code does 
>>>>>>> not
>>>>>>> introduce significant overhead or side effects.
>>>>>>> I would appreciate if you could share more details about your 
>>>>>>> concerns, so I
>>>>>>> can better address them.
>>>>>>
>>>>>> Requiring a busy wait like this is a sign of a design problem.
>>>>>>
>>>>>> There needs to be a way to setup the incoming socket listener
>>>>>> without resorting to busy waiting - that's showing a lack of
>>>>>> synchronization.
>>>>>
>>>>> How is this a design problem?  If I start a program that creates a 
>>>>> listening unix
>>>>> domain socket, I cannot attempt to connect to it until the socket 
>>>>> is created and
>>>>> listening. Clients face the same issue when starting qemu and 
>>>>> connecting to the
>>>>> monitor socket.
>>>>
>>>> Yes, the monitor has the same conceptual problem, and this caused 
>>>> problems
>>>> for libvirt starting QEMU for many years.
>>>>
>>>> With the busy wait you risk looping forever if the child (target) QEMU
>>>> already exited for some reason without ever creating the socket. You
>>>> can mitigate this by using 'kill($PID, 0)' in the loop and looking
>>>> for -ERSCH, but this only works if you know the pid involved.
>>>>
>>>> One option is to use 'daemonize' such that when the parent sees the 
>>>> initial
>>>> QEMU process leader exit, the parent has a guarantee that the 
>>>> daemonized
>>>> QEMU already has the UNIX listener open, and any failure indicates 
>>>> QEMU
>>>> already quit.
>>>>
>>>> The other option is to use FD passing such that QEMU is not 
>>>> responsible
>>>> for opening the listener socket - it gets passed a pre-opened listener
>>>> FD, so the parent has a guarantee it can successfull connect 
>>>> immediately
>>>> and any failure indicates QEMU already quit.
>>>>
>>>> For the tests, passing a pre-opened UNIX socket FD could work, but I'm
>>>> still curious why this is only a problem for the CPR tests, and not
>>>> the other migration tests which don't use 'defer'. What has made CPR
>>>> special to expose a race ?
>>>
>>> For normal migration, target qemu listens on the migration socket, 
>>> then listens
>>> on the monitor.  After the client connects to the monitor (waiting 
>>> for it to appear
>>> as needed), them the migration socket already exists.
>>>
>>> For cpr, target qemu creates the cpr socket and listens before the 
>>> monitor is
>>> started, which is necessary because cpr state is needed before 
>>> backends or
>>> devices are created.
>>>
>>> A few months back I sent a series to start the monitor first (I 
>>> think I called
>>> it the precreate phase), but it was derailed over discussions about 
>>> allowing
>>> qemu to start with no arguments and be configured exclusively via 
>>> the monitor.
>>>
>>> - Steve
>>
>> Thank you for sharing your thoughts.
>>
>> I agree that busy waiting is not ideal.
>> However, considering the timing of when target QEMU creates and 
>> begins listening on the socket,
>> I think there is currently no reliable way for the host to check the 
>> socket's listening state.
>> This also implies that FD passing is not a viable option in this case.
>>
>> As for the 'defer' option in qtest,
>> it doesn't cause race-condition issues because the target enters the 
>> listening state during the option processing.
>>
>> Of course, to address this issue,
>> I could create a wait_for_socket() function similar to 
>> wait_for_serial() in qtest framework.
>> Since the socket might already be created, I cannot simply wait for 
>> the file to appear using file system notification APIs like inotify,
>> so busy-waiting would still be necessary.
>>
>> I would appreciate hearing any further thoughts you might have on this.
>
> The easiest solution, with no interface changes, is adding 
> wait_for_socket() in qtest,
> with this addition from Daniel:
>
>   "With the busy wait you risk looping forever if the child (target) QEMU
>   already exited for some reason without ever creating the socket. You
>   can mitigate this by using 'kill($PID, 0)' in the loop and looking
>   for -ERSCH, but this only works if you know the pid involved."
>
> Daniel also suggested:
>   "For the tests, passing a pre-opened UNIX socket FD could work"
>
> Note we can not use any of the standard chardev options to specify 
> such a socket,
> because the cpr socket is created before chardevs are created.
>
> Perhaps we could specify the fd in an extension of the 
> MigrationChannel MigrationAddress.
> { 'union': 'MigrationAddress',
>   'base': { 'transport' : 'MigrationAddressType'},
>   'discriminator': 'transport',
>   'data': {
>     'socket': 'SocketAddress',
>     'exec': 'MigrationExecCommand',
>     'rdma': 'InetSocketAddress',
>     'file': 'FileMigrationArgs',
>     'fd':   'FdMigrationArgs' } }           <-- add this
>
> That would be useful for all clients, but this is asking a lot from you,
> when you are just trying to fix the tests.
>
> - Steve
>
>
Thanks for the feedback and suggestions.

I'll update the patch to v2 accordingly.

- Jaehoon Kim

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Daniel P. Berrangé]

On Fri, Jun 06, 2025 at 03:37:56PM -0400, Steven Sistare wrote:
> On 6/6/2025 2:06 PM, JAEHOON KIM wrote:
> > On 6/6/2025 12:04 PM, Steven Sistare wrote:
> > > On 6/6/2025 12:06 PM, Daniel P. Berrangé wrote:
> > > > On Fri, Jun 06, 2025 at 11:50:10AM -0400, Steven Sistare wrote:
> > > > > On 6/6/2025 11:43 AM, Daniel P. Berrangé wrote:
> > > > > > On Fri, Jun 06, 2025 at 10:37:28AM -0500, JAEHOON KIM wrote:
> > > > > > > On 6/6/2025 10:12 AM, Steven Sistare wrote:
> > > > > > > > On 6/6/2025 11:06 AM, JAEHOON KIM wrote:
> > > > > > > > > On 6/6/2025 9:14 AM, Steven Sistare wrote:
> > > > > > > > > > On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
> > > > > > > > > > > On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
> > > > > > > > > > > > When the source VM attempts to connect to the destination VM's Unix
> > > > > > > > > > > > domain socket(cpr.sock) during CPR transfer, the socket
> > > > > > > > > > > > file might not
> > > > > > > > > > > > yet be exist if the destination side hasn't completed the bind
> > > > > > > > > > > > operation. This can lead to connection failures when
> > > > > > > > > > > > running tests with
> > > > > > > > > > > > the qtest framework.
> > > > > > > > > > > 
> > > > > > > > > > > This sounds like a flawed test impl to me - whatever is initiating
> > > > > > > > > > > the cpr operation on the source has done so prematurely - it should
> > > > > > > > > > > ensure the dest is ready before starting the operation.
> > > > > > > > > > > 
> > > > > > > > > > > > To address this, add cpr_validate_socket_path(), which wait for the
> > > > > > > > > > > > socket file to appear. This avoids intermittent qtest
> > > > > > > > > > > > failures caused by
> > > > > > > > > > > > early connection attempts.
> > > > > > > > > > > 
> > > > > > > > > > > IMHO it is dubious to special case cpr in this way.
> > > > > > > > > > 
> > > > > > > > > > I agree with Daniel, and unfortunately it is not just a test issue;
> > > > > > > > > > every management framework that supports cpr-transfer must add logic to
> > > > > > > > > > wait for the cpr socket to appear in the target before proceeding.
> > > > > > > > > > 
> > > > > > > > > > This is analogous to waiting for the monitor socket to appear before
> > > > > > > > > > connecting to it.
> > > > > > > > > > 
> > > > > > > > > > - Steve
> > > > > > > > > 
> > > > > > > > > Thank you very much for your valuable review and feedback.
> > > > > > > > > 
> > > > > > > > > Just to clarify, the added cpr_validate_socket_path() function is
> > > > > > > > > not limited to the test framework.
> > > > > > > > > It is part of the actual CPR implementation and is intended to
> > > > > > > > > ensure correct behavior in all cases, including outside of tests.
> > > > > > > > > 
> > > > > > > > > I mentioned the qtest failure simply as an example where this issue
> > > > > > > > > became apparent.
> > > > > > > > 
> > > > > > > > Yes, I understand that you understand :)
> > > > > > > > Are you willing to move your fix to the qtest?
> > > > > > > > 
> > > > > > > > - Steve
> > > > > > > 
> > > > > > > Thank you for your question and feedback.
> > > > > > > 
> > > > > > > I agree that the issue could be addressed within the qtest framework to
> > > > > > > improve stability.
> > > > > > > 
> > > > > > > However, this socket readiness check is a fundamental part of CPR transfer
> > > > > > > process,
> > > > > > > and I believe that incorporating cpr_validate_socket_path() directly into
> > > > > > > the CPR implementation helps ensure more reliable behavior
> > > > > > > across all environments - not only during testing.
> > > > > > > 
> > > > > > > Just from my perspective, adding this logic to the CPR code does not
> > > > > > > introduce significant overhead or side effects.
> > > > > > > I would appreciate if you could share more details about your concerns, so I
> > > > > > > can better address them.
> > > > > > 
> > > > > > Requiring a busy wait like this is a sign of a design problem.
> > > > > > 
> > > > > > There needs to be a way to setup the incoming socket listener
> > > > > > without resorting to busy waiting - that's showing a lack of
> > > > > > synchronization.
> > > > > 
> > > > > How is this a design problem?  If I start a program that creates a listening unix
> > > > > domain socket, I cannot attempt to connect to it until the socket is created and
> > > > > listening. Clients face the same issue when starting qemu and connecting to the
> > > > > monitor socket.
> > > > 
> > > > Yes, the monitor has the same conceptual problem, and this caused problems
> > > > for libvirt starting QEMU for many years.
> > > > 
> > > > With the busy wait you risk looping forever if the child (target) QEMU
> > > > already exited for some reason without ever creating the socket. You
> > > > can mitigate this by using 'kill($PID, 0)' in the loop and looking
> > > > for -ERSCH, but this only works if you know the pid involved.
> > > > 
> > > > One option is to use 'daemonize' such that when the parent sees the initial
> > > > QEMU process leader exit, the parent has a guarantee that the daemonized
> > > > QEMU already has the UNIX listener open, and any failure indicates QEMU
> > > > already quit.
> > > > 
> > > > The other option is to use FD passing such that QEMU is not responsible
> > > > for opening the listener socket - it gets passed a pre-opened listener
> > > > FD, so the parent has a guarantee it can successfull connect immediately
> > > > and any failure indicates QEMU already quit.
> > > > 
> > > > For the tests, passing a pre-opened UNIX socket FD could work, but I'm
> > > > still curious why this is only a problem for the CPR tests, and not
> > > > the other migration tests which don't use 'defer'. What has made CPR
> > > > special to expose a race ?
> > > 
> > > For normal migration, target qemu listens on the migration socket, then listens
> > > on the monitor.  After the client connects to the monitor (waiting for it to appear
> > > as needed), them the migration socket already exists.
> > > 
> > > For cpr, target qemu creates the cpr socket and listens before the monitor is
> > > started, which is necessary because cpr state is needed before backends or
> > > devices are created.
> > > 
> > > A few months back I sent a series to start the monitor first (I think I called
> > > it the precreate phase), but it was derailed over discussions about allowing
> > > qemu to start with no arguments and be configured exclusively via the monitor.
> > > 
> > > - Steve
> > 
> > Thank you for sharing your thoughts.
> > 
> > I agree that busy waiting is not ideal.
> > However, considering the timing of when target QEMU creates and begins listening on the socket,
> > I think there is currently no reliable way for the host to check the socket's listening state.
> > This also implies that FD passing is not a viable option in this case.
> > 
> > As for the 'defer' option in qtest,
> > it doesn't cause race-condition issues because the target enters the listening state during the option processing.
> > 
> > Of course, to address this issue,
> > I could create a wait_for_socket() function similar to wait_for_serial() in qtest framework.
> > Since the socket might already be created, I cannot simply wait for the file to appear using file system notification APIs like inotify,
> > so busy-waiting would still be necessary.
> > 
> > I would appreciate hearing any further thoughts you might have on this.
> 
> The easiest solution, with no interface changes, is adding wait_for_socket() in qtest,
> with this addition from Daniel:
> 
>   "With the busy wait you risk looping forever if the child (target) QEMU
>   already exited for some reason without ever creating the socket. You
>   can mitigate this by using 'kill($PID, 0)' in the loop and looking
>   for -ERSCH, but this only works if you know the pid involved."
> 
> Daniel also suggested:
>   "For the tests, passing a pre-opened UNIX socket FD could work"
> 
> Note we can not use any of the standard chardev options to specify such a socket,
> because the cpr socket is created before chardevs are created.
> 
> Perhaps we could specify the fd in an extension of the MigrationChannel MigrationAddress.
> { 'union': 'MigrationAddress',
>   'base': { 'transport' : 'MigrationAddressType'},
>   'discriminator': 'transport',
>   'data': {
>     'socket': 'SocketAddress',
>     'exec': 'MigrationExecCommand',
>     'rdma': 'InetSocketAddress',
>     'file': 'FileMigrationArgs',
>     'fd':   'FdMigrationArgs' } }           <-- add this
> 
> That would be useful for all clients, but this is asking a lot from you,
> when you are just trying to fix the tests.

Note, 'SocketAddress' already has an option for declaring a FD that
represents a socket.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Steven Sistare]

On 6/9/2025 4:06 AM, Daniel P. Berrangé wrote:
> On Fri, Jun 06, 2025 at 03:37:56PM -0400, Steven Sistare wrote:
>> On 6/6/2025 2:06 PM, JAEHOON KIM wrote:
>>> On 6/6/2025 12:04 PM, Steven Sistare wrote:
>>>> On 6/6/2025 12:06 PM, Daniel P. Berrangé wrote:
>>>>> On Fri, Jun 06, 2025 at 11:50:10AM -0400, Steven Sistare wrote:
>>>>>> On 6/6/2025 11:43 AM, Daniel P. Berrangé wrote:
>>>>>>> On Fri, Jun 06, 2025 at 10:37:28AM -0500, JAEHOON KIM wrote:
>>>>>>>> On 6/6/2025 10:12 AM, Steven Sistare wrote:
>>>>>>>>> On 6/6/2025 11:06 AM, JAEHOON KIM wrote:
>>>>>>>>>> On 6/6/2025 9:14 AM, Steven Sistare wrote:
>>>>>>>>>>> On 6/6/2025 9:53 AM, Daniel P. Berrangé wrote:
>>>>>>>>>>>> On Thu, Jun 05, 2025 at 06:08:08PM -0500, Jaehoon Kim wrote:
>>>>>>>>>>>>> When the source VM attempts to connect to the destination VM's Unix
>>>>>>>>>>>>> domain socket(cpr.sock) during CPR transfer, the socket
>>>>>>>>>>>>> file might not
>>>>>>>>>>>>> yet be exist if the destination side hasn't completed the bind
>>>>>>>>>>>>> operation. This can lead to connection failures when
>>>>>>>>>>>>> running tests with
>>>>>>>>>>>>> the qtest framework.
>>>>>>>>>>>>
>>>>>>>>>>>> This sounds like a flawed test impl to me - whatever is initiating
>>>>>>>>>>>> the cpr operation on the source has done so prematurely - it should
>>>>>>>>>>>> ensure the dest is ready before starting the operation.
>>>>>>>>>>>>
>>>>>>>>>>>>> To address this, add cpr_validate_socket_path(), which wait for the
>>>>>>>>>>>>> socket file to appear. This avoids intermittent qtest
>>>>>>>>>>>>> failures caused by
>>>>>>>>>>>>> early connection attempts.
>>>>>>>>>>>>
>>>>>>>>>>>> IMHO it is dubious to special case cpr in this way.
>>>>>>>>>>>
>>>>>>>>>>> I agree with Daniel, and unfortunately it is not just a test issue;
>>>>>>>>>>> every management framework that supports cpr-transfer must add logic to
>>>>>>>>>>> wait for the cpr socket to appear in the target before proceeding.
>>>>>>>>>>>
>>>>>>>>>>> This is analogous to waiting for the monitor socket to appear before
>>>>>>>>>>> connecting to it.
>>>>>>>>>>>
>>>>>>>>>>> - Steve
>>>>>>>>>>
>>>>>>>>>> Thank you very much for your valuable review and feedback.
>>>>>>>>>>
>>>>>>>>>> Just to clarify, the added cpr_validate_socket_path() function is
>>>>>>>>>> not limited to the test framework.
>>>>>>>>>> It is part of the actual CPR implementation and is intended to
>>>>>>>>>> ensure correct behavior in all cases, including outside of tests.
>>>>>>>>>>
>>>>>>>>>> I mentioned the qtest failure simply as an example where this issue
>>>>>>>>>> became apparent.
>>>>>>>>>
>>>>>>>>> Yes, I understand that you understand :)
>>>>>>>>> Are you willing to move your fix to the qtest?
>>>>>>>>>
>>>>>>>>> - Steve
>>>>>>>>
>>>>>>>> Thank you for your question and feedback.
>>>>>>>>
>>>>>>>> I agree that the issue could be addressed within the qtest framework to
>>>>>>>> improve stability.
>>>>>>>>
>>>>>>>> However, this socket readiness check is a fundamental part of CPR transfer
>>>>>>>> process,
>>>>>>>> and I believe that incorporating cpr_validate_socket_path() directly into
>>>>>>>> the CPR implementation helps ensure more reliable behavior
>>>>>>>> across all environments - not only during testing.
>>>>>>>>
>>>>>>>> Just from my perspective, adding this logic to the CPR code does not
>>>>>>>> introduce significant overhead or side effects.
>>>>>>>> I would appreciate if you could share more details about your concerns, so I
>>>>>>>> can better address them.
>>>>>>>
>>>>>>> Requiring a busy wait like this is a sign of a design problem.
>>>>>>>
>>>>>>> There needs to be a way to setup the incoming socket listener
>>>>>>> without resorting to busy waiting - that's showing a lack of
>>>>>>> synchronization.
>>>>>>
>>>>>> How is this a design problem?  If I start a program that creates a listening unix
>>>>>> domain socket, I cannot attempt to connect to it until the socket is created and
>>>>>> listening. Clients face the same issue when starting qemu and connecting to the
>>>>>> monitor socket.
>>>>>
>>>>> Yes, the monitor has the same conceptual problem, and this caused problems
>>>>> for libvirt starting QEMU for many years.
>>>>>
>>>>> With the busy wait you risk looping forever if the child (target) QEMU
>>>>> already exited for some reason without ever creating the socket. You
>>>>> can mitigate this by using 'kill($PID, 0)' in the loop and looking
>>>>> for -ERSCH, but this only works if you know the pid involved.
>>>>>
>>>>> One option is to use 'daemonize' such that when the parent sees the initial
>>>>> QEMU process leader exit, the parent has a guarantee that the daemonized
>>>>> QEMU already has the UNIX listener open, and any failure indicates QEMU
>>>>> already quit.
>>>>>
>>>>> The other option is to use FD passing such that QEMU is not responsible
>>>>> for opening the listener socket - it gets passed a pre-opened listener
>>>>> FD, so the parent has a guarantee it can successfull connect immediately
>>>>> and any failure indicates QEMU already quit.
>>>>>
>>>>> For the tests, passing a pre-opened UNIX socket FD could work, but I'm
>>>>> still curious why this is only a problem for the CPR tests, and not
>>>>> the other migration tests which don't use 'defer'. What has made CPR
>>>>> special to expose a race ?
>>>>
>>>> For normal migration, target qemu listens on the migration socket, then listens
>>>> on the monitor.  After the client connects to the monitor (waiting for it to appear
>>>> as needed), them the migration socket already exists.
>>>>
>>>> For cpr, target qemu creates the cpr socket and listens before the monitor is
>>>> started, which is necessary because cpr state is needed before backends or
>>>> devices are created.
>>>>
>>>> A few months back I sent a series to start the monitor first (I think I called
>>>> it the precreate phase), but it was derailed over discussions about allowing
>>>> qemu to start with no arguments and be configured exclusively via the monitor.
>>>>
>>>> - Steve
>>>
>>> Thank you for sharing your thoughts.
>>>
>>> I agree that busy waiting is not ideal.
>>> However, considering the timing of when target QEMU creates and begins listening on the socket,
>>> I think there is currently no reliable way for the host to check the socket's listening state.
>>> This also implies that FD passing is not a viable option in this case.
>>>
>>> As for the 'defer' option in qtest,
>>> it doesn't cause race-condition issues because the target enters the listening state during the option processing.
>>>
>>> Of course, to address this issue,
>>> I could create a wait_for_socket() function similar to wait_for_serial() in qtest framework.
>>> Since the socket might already be created, I cannot simply wait for the file to appear using file system notification APIs like inotify,
>>> so busy-waiting would still be necessary.
>>>
>>> I would appreciate hearing any further thoughts you might have on this.
>>
>> The easiest solution, with no interface changes, is adding wait_for_socket() in qtest,
>> with this addition from Daniel:
>>
>>    "With the busy wait you risk looping forever if the child (target) QEMU
>>    already exited for some reason without ever creating the socket. You
>>    can mitigate this by using 'kill($PID, 0)' in the loop and looking
>>    for -ERSCH, but this only works if you know the pid involved."
>>
>> Daniel also suggested:
>>    "For the tests, passing a pre-opened UNIX socket FD could work"
>>
>> Note we can not use any of the standard chardev options to specify such a socket,
>> because the cpr socket is created before chardevs are created.
>>
>> Perhaps we could specify the fd in an extension of the MigrationChannel MigrationAddress.
>> { 'union': 'MigrationAddress',
>>    'base': { 'transport' : 'MigrationAddressType'},
>>    'discriminator': 'transport',
>>    'data': {
>>      'socket': 'SocketAddress',
>>      'exec': 'MigrationExecCommand',
>>      'rdma': 'InetSocketAddress',
>>      'file': 'FileMigrationArgs',
>>      'fd':   'FdMigrationArgs' } }           <-- add this
>>
>> That would be useful for all clients, but this is asking a lot from you,
>> when you are just trying to fix the tests.
> 
> Note, 'SocketAddress' already has an option for declaring a FD that
> represents a socket.

Yes, but if I understand, you proposed passing an fd that represents a
pre-listened socket, which requires target qemu to accept() first.  The
existing FdSocketAddress is ready to read.  We could add a boolean to enable
the new behavior.

Or maybe this is better, no new interfaces required:  qtest creates a
socketpair(AF_UNIX), sends GetFd to old qemu with the writer fd, and
specifies the reader fd in FdSocketAddress during the fork/exec of target qemu.

- Steve

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Daniel P. Berrangé]

On Mon, Jun 09, 2025 at 09:12:27AM -0400, Steven Sistare wrote:
> On 6/9/2025 4:06 AM, Daniel P. Berrangé wrote:
> > On Fri, Jun 06, 2025 at 03:37:56PM -0400, Steven Sistare wrote:
> > > 
> > > The easiest solution, with no interface changes, is adding wait_for_socket() in qtest,
> > > with this addition from Daniel:
> > > 
> > >    "With the busy wait you risk looping forever if the child (target) QEMU
> > >    already exited for some reason without ever creating the socket. You
> > >    can mitigate this by using 'kill($PID, 0)' in the loop and looking
> > >    for -ERSCH, but this only works if you know the pid involved."
> > > 
> > > Daniel also suggested:
> > >    "For the tests, passing a pre-opened UNIX socket FD could work"
> > > 
> > > Note we can not use any of the standard chardev options to specify such a socket,
> > > because the cpr socket is created before chardevs are created.
> > > 
> > > Perhaps we could specify the fd in an extension of the MigrationChannel MigrationAddress.
> > > { 'union': 'MigrationAddress',
> > >    'base': { 'transport' : 'MigrationAddressType'},
> > >    'discriminator': 'transport',
> > >    'data': {
> > >      'socket': 'SocketAddress',
> > >      'exec': 'MigrationExecCommand',
> > >      'rdma': 'InetSocketAddress',
> > >      'file': 'FileMigrationArgs',
> > >      'fd':   'FdMigrationArgs' } }           <-- add this
> > > 
> > > That would be useful for all clients, but this is asking a lot from you,
> > > when you are just trying to fix the tests.
> > 
> > Note, 'SocketAddress' already has an option for declaring a FD that
> > represents a socket.
> 
> Yes, but if I understand, you proposed passing an fd that represents a
> pre-listened socket, which requires target qemu to accept() first.  The
> existing FdSocketAddress is ready to read.  We could add a boolean to enable
> the new behavior.

It can do both actually - it depends on what APIs the QEMU uses the
SocketAddress with.

If it is used with qio_channel_socket_connect* the FD must be an
active peer connection.

If it is used with qio_channel_socket_listen*/qio_net_listener* the
FD must be listener socket.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Steven Sistare]

On 6/9/2025 9:20 AM, Daniel P. Berrangé wrote:
> On Mon, Jun 09, 2025 at 09:12:27AM -0400, Steven Sistare wrote:
>> On 6/9/2025 4:06 AM, Daniel P. Berrangé wrote:
>>> On Fri, Jun 06, 2025 at 03:37:56PM -0400, Steven Sistare wrote:
>>>>
>>>> The easiest solution, with no interface changes, is adding wait_for_socket() in qtest,
>>>> with this addition from Daniel:
>>>>
>>>>     "With the busy wait you risk looping forever if the child (target) QEMU
>>>>     already exited for some reason without ever creating the socket. You
>>>>     can mitigate this by using 'kill($PID, 0)' in the loop and looking
>>>>     for -ERSCH, but this only works if you know the pid involved."
>>>>
>>>> Daniel also suggested:
>>>>     "For the tests, passing a pre-opened UNIX socket FD could work"
>>>>
>>>> Note we can not use any of the standard chardev options to specify such a socket,
>>>> because the cpr socket is created before chardevs are created.
>>>>
>>>> Perhaps we could specify the fd in an extension of the MigrationChannel MigrationAddress.
>>>> { 'union': 'MigrationAddress',
>>>>     'base': { 'transport' : 'MigrationAddressType'},
>>>>     'discriminator': 'transport',
>>>>     'data': {
>>>>       'socket': 'SocketAddress',
>>>>       'exec': 'MigrationExecCommand',
>>>>       'rdma': 'InetSocketAddress',
>>>>       'file': 'FileMigrationArgs',
>>>>       'fd':   'FdMigrationArgs' } }           <-- add this
>>>>
>>>> That would be useful for all clients, but this is asking a lot from you,
>>>> when you are just trying to fix the tests.
>>>
>>> Note, 'SocketAddress' already has an option for declaring a FD that
>>> represents a socket.
>>
>> Yes, but if I understand, you proposed passing an fd that represents a
>> pre-listened socket, which requires target qemu to accept() first.  The
>> existing FdSocketAddress is ready to read.  We could add a boolean to enable
>> the new behavior.
> 
> It can do both actually - it depends on what APIs the QEMU uses the
> SocketAddress with.
> 
> If it is used with qio_channel_socket_connect* the FD must be an
> active peer connection.
> 
> If it is used with qio_channel_socket_listen*/qio_net_listener* the
> FD must be listener socket.

Fair enough.  cpr currently listens here, and we could add a case for the FD:

   QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
   {
     MigrationAddress *addr = channel->addr;

     if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET &&
         addr->u.socket.type == SOCKET_ADDRESS_TYPE_UNIX) {
         ...
         g_autoptr(QIONetListener) listener = qio_net_listener_new();

Or to use my socketpair() suggestion, that function would also need changes,
calling qio_channel_socket_connect.

Which do you think is better for clients -- socketpair or pre-listened?

- Steve

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[JAEHOON KIM]

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


On 6/9/2025 8:39 AM, Steven Sistare wrote:
> On 6/9/2025 9:20 AM, Daniel P. Berrangé wrote:
>> On Mon, Jun 09, 2025 at 09:12:27AM -0400, Steven Sistare wrote:
>>> On 6/9/2025 4:06 AM, Daniel P. Berrangé wrote:
>>>> On Fri, Jun 06, 2025 at 03:37:56PM -0400, Steven Sistare wrote:
>>>>>
>>>>> The easiest solution, with no interface changes, is adding 
>>>>> wait_for_socket() in qtest,
>>>>> with this addition from Daniel:
>>>>>
>>>>>     "With the busy wait you risk looping forever if the child 
>>>>> (target) QEMU
>>>>>     already exited for some reason without ever creating the 
>>>>> socket. You
>>>>>     can mitigate this by using 'kill($PID, 0)' in the loop and 
>>>>> looking
>>>>>     for -ERSCH, but this only works if you know the pid involved."
>>>>>
>>>>> Daniel also suggested:
>>>>>     "For the tests, passing a pre-opened UNIX socket FD could work"
>>>>>
>>>>> Note we can not use any of the standard chardev options to specify 
>>>>> such a socket,
>>>>> because the cpr socket is created before chardevs are created.
>>>>>
>>>>> Perhaps we could specify the fd in an extension of the 
>>>>> MigrationChannel MigrationAddress.
>>>>> { 'union': 'MigrationAddress',
>>>>>     'base': { 'transport' : 'MigrationAddressType'},
>>>>>     'discriminator': 'transport',
>>>>>     'data': {
>>>>>       'socket': 'SocketAddress',
>>>>>       'exec': 'MigrationExecCommand',
>>>>>       'rdma': 'InetSocketAddress',
>>>>>       'file': 'FileMigrationArgs',
>>>>>       'fd':   'FdMigrationArgs' } }           <-- add this
>>>>>
>>>>> That would be useful for all clients, but this is asking a lot 
>>>>> from you,
>>>>> when you are just trying to fix the tests.
>>>>
>>>> Note, 'SocketAddress' already has an option for declaring a FD that
>>>> represents a socket.
>>>
>>> Yes, but if I understand, you proposed passing an fd that represents a
>>> pre-listened socket, which requires target qemu to accept() first.  The
>>> existing FdSocketAddress is ready to read.  We could add a boolean 
>>> to enable
>>> the new behavior.
>>
>> It can do both actually - it depends on what APIs the QEMU uses the
>> SocketAddress with.
>>
>> If it is used with qio_channel_socket_connect* the FD must be an
>> active peer connection.
>>
>> If it is used with qio_channel_socket_listen*/qio_net_listener* the
>> FD must be listener socket.
>
> Fair enough.  cpr currently listens here, and we could add a case for 
> the FD:
>
>   QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
>   {
>     MigrationAddress *addr = channel->addr;
>
>     if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET &&
>         addr->u.socket.type == SOCKET_ADDRESS_TYPE_UNIX) {
>         ...
>         g_autoptr(QIONetListener) listener = qio_net_listener_new();
>
> Or to use my socketpair() suggestion, that function would also need 
> changes,
> calling qio_channel_socket_connect.
>
> Which do you think is better for clients -- socketpair or pre-listened?
>
> - Steve
>
>
Using a socketpair() to create two descriptors and passing them to the 
old QEMU and the target QEMU, instead of relying on cpr.sock, seems like 
a more efficient approach overall.
However, adopting this method would require changes to the logic in 
cpr_transfer_xxx as Steve mentioned.

Based on your suggestions, it seems that I may need to adjust the 
direction of my next patch.
Initially, I was simply planning to add logic that waits for the 
creation of |cpr.sock| and the target QEMU process state.
However, considering your input, it might be better to reconsider the 
approach.

- Jaehoo Kim

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

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Daniel P. Berrangé]

On Mon, Jun 09, 2025 at 09:39:48AM -0400, Steven Sistare wrote:
> On 6/9/2025 9:20 AM, Daniel P. Berrangé wrote:
> > On Mon, Jun 09, 2025 at 09:12:27AM -0400, Steven Sistare wrote:
> > > On 6/9/2025 4:06 AM, Daniel P. Berrangé wrote:
> > > > On Fri, Jun 06, 2025 at 03:37:56PM -0400, Steven Sistare wrote:
> > > > > 
> > > > > The easiest solution, with no interface changes, is adding wait_for_socket() in qtest,
> > > > > with this addition from Daniel:
> > > > > 
> > > > >     "With the busy wait you risk looping forever if the child (target) QEMU
> > > > >     already exited for some reason without ever creating the socket. You
> > > > >     can mitigate this by using 'kill($PID, 0)' in the loop and looking
> > > > >     for -ERSCH, but this only works if you know the pid involved."
> > > > > 
> > > > > Daniel also suggested:
> > > > >     "For the tests, passing a pre-opened UNIX socket FD could work"
> > > > > 
> > > > > Note we can not use any of the standard chardev options to specify such a socket,
> > > > > because the cpr socket is created before chardevs are created.
> > > > > 
> > > > > Perhaps we could specify the fd in an extension of the MigrationChannel MigrationAddress.
> > > > > { 'union': 'MigrationAddress',
> > > > >     'base': { 'transport' : 'MigrationAddressType'},
> > > > >     'discriminator': 'transport',
> > > > >     'data': {
> > > > >       'socket': 'SocketAddress',
> > > > >       'exec': 'MigrationExecCommand',
> > > > >       'rdma': 'InetSocketAddress',
> > > > >       'file': 'FileMigrationArgs',
> > > > >       'fd':   'FdMigrationArgs' } }           <-- add this
> > > > > 
> > > > > That would be useful for all clients, but this is asking a lot from you,
> > > > > when you are just trying to fix the tests.
> > > > 
> > > > Note, 'SocketAddress' already has an option for declaring a FD that
> > > > represents a socket.
> > > 
> > > Yes, but if I understand, you proposed passing an fd that represents a
> > > pre-listened socket, which requires target qemu to accept() first.  The
> > > existing FdSocketAddress is ready to read.  We could add a boolean to enable
> > > the new behavior.
> > 
> > It can do both actually - it depends on what APIs the QEMU uses the
> > SocketAddress with.
> > 
> > If it is used with qio_channel_socket_connect* the FD must be an
> > active peer connection.
> > 
> > If it is used with qio_channel_socket_listen*/qio_net_listener* the
> > FD must be listener socket.
> 
> Fair enough.  cpr currently listens here, and we could add a case for the FD:
> 
>   QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
>   {
>     MigrationAddress *addr = channel->addr;
> 
>     if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET &&
>         addr->u.socket.type == SOCKET_ADDRESS_TYPE_UNIX) {
>         ...
>         g_autoptr(QIONetListener) listener = qio_net_listener_new();
> 
> Or to use my socketpair() suggestion, that function would also need changes,
> calling qio_channel_socket_connect.
> 
> Which do you think is better for clients -- socketpair or pre-listened?

Please just use the existing SocketAddress functionality, as that's used
throughout QEMU - a special case with socketpair for migration is not
desirable.

The SocketAddress stuff is what libvirt's used for many years now to
address the race condition with QMP listeners.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[JAEHOON KIM]

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


On 6/9/2025 8:50 AM, Daniel P. Berrangé wrote:
> On Mon, Jun 09, 2025 at 09:39:48AM -0400, Steven Sistare wrote:
>> On 6/9/2025 9:20 AM, Daniel P. Berrangé wrote:
>>> On Mon, Jun 09, 2025 at 09:12:27AM -0400, Steven Sistare wrote:
>>>> On 6/9/2025 4:06 AM, Daniel P. Berrangé wrote:
>>>>> On Fri, Jun 06, 2025 at 03:37:56PM -0400, Steven Sistare wrote:
>>>>>> The easiest solution, with no interface changes, is adding wait_for_socket() in qtest,
>>>>>> with this addition from Daniel:
>>>>>>
>>>>>>      "With the busy wait you risk looping forever if the child (target) QEMU
>>>>>>      already exited for some reason without ever creating the socket. You
>>>>>>      can mitigate this by using 'kill($PID, 0)' in the loop and looking
>>>>>>      for -ERSCH, but this only works if you know the pid involved."
>>>>>>
>>>>>> Daniel also suggested:
>>>>>>      "For the tests, passing a pre-opened UNIX socket FD could work"
>>>>>>
>>>>>> Note we can not use any of the standard chardev options to specify such a socket,
>>>>>> because the cpr socket is created before chardevs are created.
>>>>>>
>>>>>> Perhaps we could specify the fd in an extension of the MigrationChannel MigrationAddress.
>>>>>> { 'union': 'MigrationAddress',
>>>>>>      'base': { 'transport' : 'MigrationAddressType'},
>>>>>>      'discriminator': 'transport',
>>>>>>      'data': {
>>>>>>        'socket': 'SocketAddress',
>>>>>>        'exec': 'MigrationExecCommand',
>>>>>>        'rdma': 'InetSocketAddress',
>>>>>>        'file': 'FileMigrationArgs',
>>>>>>        'fd':   'FdMigrationArgs' } }           <-- add this
>>>>>>
>>>>>> That would be useful for all clients, but this is asking a lot from you,
>>>>>> when you are just trying to fix the tests.
>>>>> Note, 'SocketAddress' already has an option for declaring a FD that
>>>>> represents a socket.
>>>> Yes, but if I understand, you proposed passing an fd that represents a
>>>> pre-listened socket, which requires target qemu to accept() first.  The
>>>> existing FdSocketAddress is ready to read.  We could add a boolean to enable
>>>> the new behavior.
>>> It can do both actually - it depends on what APIs the QEMU uses the
>>> SocketAddress with.
>>>
>>> If it is used with qio_channel_socket_connect* the FD must be an
>>> active peer connection.
>>>
>>> If it is used with qio_channel_socket_listen*/qio_net_listener* the
>>> FD must be listener socket.
>> Fair enough.  cpr currently listens here, and we could add a case for the FD:
>>
>>    QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
>>    {
>>      MigrationAddress *addr = channel->addr;
>>
>>      if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET &&
>>          addr->u.socket.type == SOCKET_ADDRESS_TYPE_UNIX) {
>>          ...
>>          g_autoptr(QIONetListener) listener = qio_net_listener_new();
>>
>> Or to use my socketpair() suggestion, that function would also need changes,
>> calling qio_channel_socket_connect.
>>
>> Which do you think is better for clients -- socketpair or pre-listened?
> Please just use the existing SocketAddress functionality, as that's used
> throughout QEMU - a special case with socketpair for migration is not
> desirable.
>
> The SocketAddress stuff is what libvirt's used for many years now to
> address the race condition with QMP listeners.
>
> With regards,
> Daniel

Dear Daniel and Steve,

Thank you both for your valuable insights.

To clarify regarding the socket handling approach:
If I do not use socketpair() and instead pass a pre-listened FD to the target, which then calls accept(),
it seems this could mitigate some race condition. However, isn't there still a risk that the old QEMU might try to
connect before the target QEMU calls accept(), thereby resulting in the same race condition?

If I only consider the qtest environment, it seems to me that waiting for the target to create cpr.sock inside qtest framework
- along with checking the process status {kill(pid,0)} - might be the most efficient way to handle this.

I checked and found that the QTestState structure already has a "pid_t qemu_pid" field,
so it should be straightforward to check the target's PID.

 From your experience,
which approach do you consider the most effective to solve the current race condition issue?

I would appreciate your thoughts.

- Jaehoon Kim

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

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[Daniel P. Berrangé]

On Mon, Jun 09, 2025 at 09:54:02AM -0500, JAEHOON KIM wrote:
> 
> On 6/9/2025 8:50 AM, Daniel P. Berrangé wrote:
> > On Mon, Jun 09, 2025 at 09:39:48AM -0400, Steven Sistare wrote:
> > > On 6/9/2025 9:20 AM, Daniel P. Berrangé wrote:
> > > > On Mon, Jun 09, 2025 at 09:12:27AM -0400, Steven Sistare wrote:
> > > > > On 6/9/2025 4:06 AM, Daniel P. Berrangé wrote:
> > > > > > On Fri, Jun 06, 2025 at 03:37:56PM -0400, Steven Sistare wrote:
> > > > > > > The easiest solution, with no interface changes, is adding wait_for_socket() in qtest,
> > > > > > > with this addition from Daniel:
> > > > > > > 
> > > > > > >      "With the busy wait you risk looping forever if the child (target) QEMU
> > > > > > >      already exited for some reason without ever creating the socket. You
> > > > > > >      can mitigate this by using 'kill($PID, 0)' in the loop and looking
> > > > > > >      for -ERSCH, but this only works if you know the pid involved."
> > > > > > > 
> > > > > > > Daniel also suggested:
> > > > > > >      "For the tests, passing a pre-opened UNIX socket FD could work"
> > > > > > > 
> > > > > > > Note we can not use any of the standard chardev options to specify such a socket,
> > > > > > > because the cpr socket is created before chardevs are created.
> > > > > > > 
> > > > > > > Perhaps we could specify the fd in an extension of the MigrationChannel MigrationAddress.
> > > > > > > { 'union': 'MigrationAddress',
> > > > > > >      'base': { 'transport' : 'MigrationAddressType'},
> > > > > > >      'discriminator': 'transport',
> > > > > > >      'data': {
> > > > > > >        'socket': 'SocketAddress',
> > > > > > >        'exec': 'MigrationExecCommand',
> > > > > > >        'rdma': 'InetSocketAddress',
> > > > > > >        'file': 'FileMigrationArgs',
> > > > > > >        'fd':   'FdMigrationArgs' } }           <-- add this
> > > > > > > 
> > > > > > > That would be useful for all clients, but this is asking a lot from you,
> > > > > > > when you are just trying to fix the tests.
> > > > > > Note, 'SocketAddress' already has an option for declaring a FD that
> > > > > > represents a socket.
> > > > > Yes, but if I understand, you proposed passing an fd that represents a
> > > > > pre-listened socket, which requires target qemu to accept() first.  The
> > > > > existing FdSocketAddress is ready to read.  We could add a boolean to enable
> > > > > the new behavior.
> > > > It can do both actually - it depends on what APIs the QEMU uses the
> > > > SocketAddress with.
> > > > 
> > > > If it is used with qio_channel_socket_connect* the FD must be an
> > > > active peer connection.
> > > > 
> > > > If it is used with qio_channel_socket_listen*/qio_net_listener* the
> > > > FD must be listener socket.
> > > Fair enough.  cpr currently listens here, and we could add a case for the FD:
> > > 
> > >    QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
> > >    {
> > >      MigrationAddress *addr = channel->addr;
> > > 
> > >      if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET &&
> > >          addr->u.socket.type == SOCKET_ADDRESS_TYPE_UNIX) {
> > >          ...
> > >          g_autoptr(QIONetListener) listener = qio_net_listener_new();
> > > 
> > > Or to use my socketpair() suggestion, that function would also need changes,
> > > calling qio_channel_socket_connect.
> > > 
> > > Which do you think is better for clients -- socketpair or pre-listened?
> > Please just use the existing SocketAddress functionality, as that's used
> > throughout QEMU - a special case with socketpair for migration is not
> > desirable.
> > 
> > The SocketAddress stuff is what libvirt's used for many years now to
> > address the race condition with QMP listeners.
> 
> Dear Daniel and Steve,
> 
> Thank you both for your valuable insights.
> 
> To clarify regarding the socket handling approach:
> If I do not use socketpair() and instead pass a pre-listened FD to the target, which then calls accept(),
> it seems this could mitigate some race condition. However, isn't there still a risk that the old QEMU might try to
> connect before the target QEMU calls accept(), thereby resulting in the same race condition?

No, that's fine. The kernel will queue all incoming connections
until accept() is called. So essentially the source QEMU connect()
will get blocked until dst QEMU accept()s, which is exactly the
semantics we want.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v1] migration: Wait for cpr.sock file to appear before connecting

[JAEHOON KIM]

On 6/9/2025 9:57 AM, Daniel P. Berrangé wrote:
> On Mon, Jun 09, 2025 at 09:54:02AM -0500, JAEHOON KIM wrote:
>> On 6/9/2025 8:50 AM, Daniel P. Berrangé wrote:
>>> On Mon, Jun 09, 2025 at 09:39:48AM -0400, Steven Sistare wrote:
>>>> On 6/9/2025 9:20 AM, Daniel P. Berrangé wrote:
>>>>> On Mon, Jun 09, 2025 at 09:12:27AM -0400, Steven Sistare wrote:
>>>>>> On 6/9/2025 4:06 AM, Daniel P. Berrangé wrote:
>>>>>>> On Fri, Jun 06, 2025 at 03:37:56PM -0400, Steven Sistare wrote:
>>>>>>>> The easiest solution, with no interface changes, is adding wait_for_socket() in qtest,
>>>>>>>> with this addition from Daniel:
>>>>>>>>
>>>>>>>>       "With the busy wait you risk looping forever if the child (target) QEMU
>>>>>>>>       already exited for some reason without ever creating the socket. You
>>>>>>>>       can mitigate this by using 'kill($PID, 0)' in the loop and looking
>>>>>>>>       for -ERSCH, but this only works if you know the pid involved."
>>>>>>>>
>>>>>>>> Daniel also suggested:
>>>>>>>>       "For the tests, passing a pre-opened UNIX socket FD could work"
>>>>>>>>
>>>>>>>> Note we can not use any of the standard chardev options to specify such a socket,
>>>>>>>> because the cpr socket is created before chardevs are created.
>>>>>>>>
>>>>>>>> Perhaps we could specify the fd in an extension of the MigrationChannel MigrationAddress.
>>>>>>>> { 'union': 'MigrationAddress',
>>>>>>>>       'base': { 'transport' : 'MigrationAddressType'},
>>>>>>>>       'discriminator': 'transport',
>>>>>>>>       'data': {
>>>>>>>>         'socket': 'SocketAddress',
>>>>>>>>         'exec': 'MigrationExecCommand',
>>>>>>>>         'rdma': 'InetSocketAddress',
>>>>>>>>         'file': 'FileMigrationArgs',
>>>>>>>>         'fd':   'FdMigrationArgs' } }           <-- add this
>>>>>>>>
>>>>>>>> That would be useful for all clients, but this is asking a lot from you,
>>>>>>>> when you are just trying to fix the tests.
>>>>>>> Note, 'SocketAddress' already has an option for declaring a FD that
>>>>>>> represents a socket.
>>>>>> Yes, but if I understand, you proposed passing an fd that represents a
>>>>>> pre-listened socket, which requires target qemu to accept() first.  The
>>>>>> existing FdSocketAddress is ready to read.  We could add a boolean to enable
>>>>>> the new behavior.
>>>>> It can do both actually - it depends on what APIs the QEMU uses the
>>>>> SocketAddress with.
>>>>>
>>>>> If it is used with qio_channel_socket_connect* the FD must be an
>>>>> active peer connection.
>>>>>
>>>>> If it is used with qio_channel_socket_listen*/qio_net_listener* the
>>>>> FD must be listener socket.
>>>> Fair enough.  cpr currently listens here, and we could add a case for the FD:
>>>>
>>>>     QEMUFile *cpr_transfer_input(MigrationChannel *channel, Error **errp)
>>>>     {
>>>>       MigrationAddress *addr = channel->addr;
>>>>
>>>>       if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET &&
>>>>           addr->u.socket.type == SOCKET_ADDRESS_TYPE_UNIX) {
>>>>           ...
>>>>           g_autoptr(QIONetListener) listener = qio_net_listener_new();
>>>>
>>>> Or to use my socketpair() suggestion, that function would also need changes,
>>>> calling qio_channel_socket_connect.
>>>>
>>>> Which do you think is better for clients -- socketpair or pre-listened?
>>> Please just use the existing SocketAddress functionality, as that's used
>>> throughout QEMU - a special case with socketpair for migration is not
>>> desirable.
>>>
>>> The SocketAddress stuff is what libvirt's used for many years now to
>>> address the race condition with QMP listeners.
>> Dear Daniel and Steve,
>>
>> Thank you both for your valuable insights.
>>
>> To clarify regarding the socket handling approach:
>> If I do not use socketpair() and instead pass a pre-listened FD to the target, which then calls accept(),
>> it seems this could mitigate some race condition. However, isn't there still a risk that the old QEMU might try to
>> connect before the target QEMU calls accept(), thereby resulting in the same race condition?
> No, that's fine. The kernel will queue all incoming connections
> until accept() is called. So essentially the source QEMU connect()
> will get blocked until dst QEMU accept()s, which is exactly the
> semantics we want.
>
> With regards,
> Daniel
Thank you for the clarification.

If, as you said, the kernel queues incoming connections until accept() 
is called, then the race condition should not occur.
I'll take this approach into consideration.

- Jaehoon Kim.