Re: [Xen-devel] [PATCH v2 07/10] libxl: add Qdisk backend launch helper

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v2 07/10] libxl: add Qdisk backend launch 
helper"):
> Current Qemu launch functions in libxl require the usage of data
> structures only avaialbe on domain creation. All this information is
> not need in order to launch a Qemu instance to serve Qdisk backends,
> so introduce a new simplified helper that can be used to launch
> Qemu/Qdisk, that will be used to launch Qdisk in driver domains.

I think the principle here is good, and the implementation is pretty
good.


Looking for code duplication:

+void libxl__spawn_qdisk_backend(libxl__egc *egc, libxl__spawn_qdisk_state *sqs)
...
> +    libxl_create_logfile(CTX, GCSPRINTF("qdisk-%u", domid), &logfile);
> +    logfile_w = open(logfile, O_WRONLY|O_CREAT|O_APPEND, 0644);
> +    if (logfile_w < 0) {
> +        free(logfile);
> +        rc = logfile_w;
> +        goto error;
> +    }
> +    free(logfile);
> +    null = open("/dev/null", O_RDONLY);

This is very like similar code in libxl__spawn_local_dm and could
perhaps profitably be factored out into a function.

> +static void qdisk_confirm(libxl__egc *egc, libxl__spawn_state *spawn,
> +                          const char *xsdata)
> +{
> +    STATE_AO_GC(spawn->ao);
> +
> +    if (!xsdata)
> +        return;
> +
> +    if (strcmp(xsdata, "running"))
> +        return;
> +
> +    libxl__spawn_initiate_detach(gc, spawn);
> +}

This is practically identical to device_model_confirm.  Indeed, if you
delete the unneeded line in device_model_confirm for getting the
(unused) dmss, I think it's entirely identical.

> +static void qdisk_startup_failed(libxl__egc *egc,
> +                                 libxl__spawn_state *spawn)
> +{
> +    libxl__spawn_qdisk_state *sqs = CONTAINER_OF(spawn, *sqs, spawn);
> +    sqs->callback(egc, sqs, ERROR_FAIL);
> +}

These are similar to the callbacks used by libxl__spawn_local_dm.

If you used a libxl__dm_spawn_state (even if mostly full of null
pointers) for the sqs, instead of its own type, you could abolish
these separate callbacks and rely on device_model_spawn_outcome to
DTRT I think.

What do you think ?


Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel