Re: [Xen-devel] [PATCH v4 30/32] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jul 27, 2018 at 03:06:12PM +0100, Anthony PERARD wrote:
> +static void dm_stopped(libxl__egc *egc, libxl__ev_qmp *ev,
> +                       const libxl__json_object *response, int rc)
> +{
> +    EGC_GC;
> +    libxl__domain_suspend_state *dsps = CONTAINER_OF(ev, *dsps, qmp);
> +    const char *const filename = dsps->dm_savefile;
> +    uint32_t const domid = ev->domid;
> +
> +    if (rc)
> +        goto out;
> +
> +    libxl__carefd_begin();
> +    ev->cfd = libxl__carefd_opened(CTX,
> +                                 open(filename, O_WRONLY | O_CREAT, 0600));
> +    if (!ev->cfd) {
> +        LOGED(ERROR, domid, "Failed to open file %s for QEMU", filename);
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    ev->callback = dm_state_fd_ready;
> +    rc = libxl__ev_qmp_send(gc, ev, "add-fd", NULL);
> +    if (rc)
> +        goto out;
> +
> +    return;
> +out:

That label would be better named 'error', because it's only used for
the error paths.

> +    if (ev->cfd) {
> +        libxl__carefd_close(ev->cfd);
> +        unlink(filename);
> +        ev->cfd = NULL;
> +    }
> +    dsps->callback_device_model_done(egc, dsps, rc);
> +}
> +
> +static void dm_state_fd_ready(libxl__egc *egc, libxl__ev_qmp *ev,
> +                              const libxl__json_object *response, int rc)
> +{
> +    EGC_GC;
> +    libxl__domain_suspend_state *dsps = CONTAINER_OF(ev, *dsps, qmp);
> +    libxl__json_object *args = NULL;
> +    const libxl__json_object *o;
> +    int fdset;
> +
> +    libxl__carefd_close(ev->cfd);
> +    ev->cfd = NULL;
> +
> +    if (rc)
> +        goto out;
> +
> +    o = libxl__json_map_get("fdset-id", response, JSON_INTEGER);
> +    if (!o) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    fdset = libxl__json_object_get_integer(o);
> +
> +    ev->callback = dm_state_saved;
> +
> +    if (qmp_ev_qemu_check_version(ev, 2, 11, 0))
> +        qmp_parameters_add_bool(gc, &args, "live", dsps->live);
> +    QMP_PARAMETERS_SPRINTF(&args, "filename", "/dev/fdset/%d", fdset);
> +    rc = libxl__ev_qmp_send(gc, ev, "xen-save-devices-state", args);

I think some comments about the logic above would be helpful (at least
for me). Do you need to pass an extra argument to 2.11.0 and higher
versions in order to live-migrate?

> +    if (rc)
> +        goto out;
> +
> +    return;
> +out:

Same here regarding the naming of the label.

> +    if (rc)
> +        unlink(dsps->dm_savefile);
> +    dsps->callback_device_model_done(egc, dsps, rc);
> +}
> +
> +static void dm_state_saved(libxl__egc *egc, libxl__ev_qmp *ev,
> +                           const libxl__json_object *response, int rc)
> +{
> +    EGC_GC;
> +    libxl__domain_suspend_state *dsps = CONTAINER_OF(ev, *dsps, qmp);
> +
> +    if (rc)
> +        unlink(dsps->dm_savefile);

Use the libxl unlink helper.

Thanks, Roger.

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