Re: [PATCH v6 8/8] tests/qtest/nvme-test: add migration test with full CQ

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Sun, Apr 19, 2026 at 03:01:39PM +0200, Alexander Mikhalitsyn wrote:
> +static void test_migrate_check_nvme(nvme_ctrl *ctrl, test_migrate_req *reqs, int num)
> +{
> +    int i;
> +
> +    for (i = 0; i < num; i++) {
> +        NvmeCqe cqe;
> +
> +        if (reqs[i].handle_cqe) {
> +            continue;
> +        }
> +
> +        cqe = nvme_wait(&ctrl->admin_sq);
> +        g_assert(nvme_is_cqe_success(&cqe));
> +
> +        g_assert_cmpint(cqe.cid, ==, reqs[i].cid);

Please check the endianness in this patch. I don't see an le16_to_cpu()
call here.

Maybe other NVMe DMA structures or registers are also not converted
to/from little endian. This test must pass on both big-endian and
little-endian hosts.

> +
> +        #define GUEST_MEM_READB(field) \
> +                            qtest_readb(ctrl->pdev->bus->qts, (uint64_t)&(field))
> +
> +        g_assert_cmpint(GUEST_MEM_READB(reqs[i].phys_identify->ieee[0]), ==, 0x0);

I was trying to figure out why phys_identify has a pointer type although
it is a guest memory address that cannot be dereferenced in C code. I
guess the reason is so that the GUEST_MEM_READB() macro can use the
address-of operator instead of explicitly calculating the offset of
ieee[0], ieee[1], etc? I found this confusing and potentially dangerous
(the compiler won't stop the pointer from being dereferenced if someone
does it by mistake).

It took more time to review the pointer trick than the straightforward
approach would have taken. I'd avoid it, but it's a question of coding
style and up to you.

> +        g_assert_cmpint(GUEST_MEM_READB(reqs[i].phys_identify->ieee[1]), ==, 0x54);
> +        g_assert_cmpint(GUEST_MEM_READB(reqs[i].phys_identify->ieee[2]), ==, 0x52);
> +
> +        #undef GUEST_MEM_READB
> +
> +        guest_free(ctrl->alloc, (uint64_t)reqs[i].phys_identify);
> +    }
> +}
> +
> +static void test_migrate(void *obj, void *data, QGuestAllocator *alloc)
> +{
> +    g_autofree gchar *tmpfs = NULL;
> +    GError *err = NULL;
> +    g_autofree gchar *mig_path;
> +    g_autofree gchar *uri;
> +    GString *dest_cmdline;
> +    QTestState *to;
> +    QDict *rsp;
> +    QNvme *nvme = obj;
> +    QPCIDevice *pdev = &nvme->dev;
> +    nvme_ctrl *ctrl;
> +    test_migrate_req test_reqs[] = {
> +        { 123, true },
> +        { 456, false },
> +        { 300, false },
> +        { 333, false }
> +    };
> +
> +    /* create temporary dir and prepare unix socket path for migration */
> +    tmpfs = g_dir_make_tmp("nvme-test-XXXXXX", &err);
> +    if (!tmpfs) {
> +        g_test_message("Can't create temporary directory in %s: %s",
> +                       g_get_tmp_dir(), err->message);
> +        g_error_free(err);
> +    }
> +    g_assert(tmpfs);
> +
> +    mig_path = g_strdup_printf("%s/socket.mig", tmpfs);
> +    uri = g_strdup_printf("unix:%s", mig_path);
> +
> +    /* enable NVMe PCI device */
> +    qpci_device_enable(pdev);
> +
> +    ctrl = g_malloc0(sizeof(*ctrl));

I don't see a g_free() call for ctrl. Use g_autofree?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]