Re: [Xen-devel] [PATCH] libxl_qmp: wait for completion of device removal

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

Hi,

Thanks for the patch. I've got some comments.

On Tue, Jul 02, 2019 at 03:46:40PM +0800, Chao Gao wrote:
> To remove a device from a domain, a qmp command is sent to qemu. But it is
> handled by qemu asychronously. Even the qmp command is claimed to be done,
> the actual handling in qemu side may happen later.
> This behavior brings two questions:
> 1. Attaching a device back to a domain right after detaching the device from
> that domain would fail with error:
> 
> libxl: error: libxl_qmp.c:341:qmp_handle_error_response: Domain 1:received an
> error message from QMP server: Duplicate ID 'pci-pt-60_00.0' for device
> 
> 2. Accesses to PCI configuration space in Qemu may overlap with later device
> reset issued by 'xl' or by pciback.
> 
> In order to avoid mentioned questions, wait for the completion of device
> removal by querying all pci devices using qmp command and ensuring the target
> device isn't listed. Only retry 5 times to avoid 'xl' potentially being 
> blocked
> by qemu.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
>  tools/libxl/libxl_qmp.c | 57 
> ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 42c8ab8..18f6126 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -1000,9 +1032,32 @@ int libxl__qmp_pci_add(libxl__gc *gc, int domid, 
> libxl_device_pci *pcidev)
>  static int qmp_device_del(libxl__gc *gc, int domid, char *id)
>  {
>      libxl__json_object *args = NULL;
> +    libxl__qmp_handler *qmp = NULL;
> +    int rc = 0;
> +
> +    qmp = libxl__qmp_initialize(gc, domid);
> +    if (!qmp)
> +        return ERROR_FAIL;
>  
>      qmp_parameters_add_string(gc, &args, "id", id);
> -    return qmp_run_command(gc, domid, "device_del", args, NULL, NULL);
> +    rc = qmp_synchronous_send(qmp, "device_del", args,
> +                              NULL, NULL, qmp->timeout);
> +    if (rc == 0) {
> +        unsigned int retry = 0;
> +
> +        do {
> +            if (qmp_synchronous_send(qmp, "query-pci", NULL,
> +                                     pci_del_callback, id, qmp->timeout) == 
> 0) {

Could you assign the return value of qmp_synchronous_send into a
variable, then check for success/error?

qmp_synchronous_send returns several possible values:
- errors, when rc < 0
- rc of the callback, which is 0 or 1 here.

If there's an error, we don't want to keep trying we probably want to
return that error.

Thanks.

> +                break;
> +            }
> +            LOGD(DEBUG, qmp->domid,
> +                 "Waiting for completion of deleting device %s", id);
> +            sleep(1);
> +        } while (retry++ < 5);
> +    }

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel