[libvirt PATCH v4 10/31] qemu: add functions to start and stop nbdkit

[Peter Krempa]

On Fri, Jan 20, 2023 at 16:03:04 -0600, Jonathon Jongsma wrote:
> Add some helper functions to build a virCommand object and run the
> nbdkit process for a given virStorageSource.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
>  src/qemu/qemu_nbdkit.c | 255 +++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_nbdkit.h |  10 ++
>  2 files changed, 265 insertions(+)

[...]

> @@ -667,6 +668,167 @@ qemuNbdkitInitStorageSource(qemuNbdkitCaps *caps,
>  }
>  
>  
> +static int
> +qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
> +                                  virCommand *cmd)
> +{
> +    g_autoptr(virURI) uri = qemuBlockStorageSourceGetURI(proc->source);
> +    g_autofree char *uristring = virURIFormat(uri);
> +
> +    /* nbdkit plugin name */
> +    virCommandAddArg(cmd, "curl");
> +    if (proc->source->protocol == VIR_STORAGE_NET_PROTOCOL_HTTP) {
> +        /* allow http to be upgraded to https via e.g. redirect */
> +        g_autofree char* protocols = g_strdup_printf("%s,%s",
> +                                                     virStorageNetProtocolTypeToString(proc->source->protocol),
> +                                                     virStorageNetProtocolTypeToString(VIR_STORAGE_NET_PROTOCOL_HTTPS));

IMO you can spell out "http,https" manually here rahter than attempting
to construct it from the convertors when the result is always the same.

> +        virCommandAddArgPair(cmd, "protocols", protocols);
> +    } else {
> +        virCommandAddArgPair(cmd, "protocols",
> +                             virStorageNetProtocolTypeToString(proc->source->protocol));
> +    }
> +    virCommandAddArgPair(cmd, "url", uristring);
> +
> +    if (proc->source->auth) {
> +        g_autoptr(virConnect) conn = virGetConnectSecret();
> +        g_autofree uint8_t *secret = NULL;
> +        size_t secretlen = 0;
> +        g_autofree char *password = NULL;
> +        int secrettype;
> +        virStorageAuthDef *authdef = proc->source->auth;
> +
> +        virCommandAddArgPair(cmd, "user",
> +                             proc->source->auth->username);
> +
> +        if ((secrettype = virSecretUsageTypeFromString(proc->source->auth->secrettype)) < 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("invalid secret type %s"),
> +                           proc->source->auth->secrettype);
> +            return -1;
> +        }
> +
> +        if (virSecretGetSecretString(conn,
> +                                     &authdef->seclookupdef,
> +                                     secrettype,
> +                                     &secret,
> +                                     &secretlen) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("failed to get auth secret for storage"));
> +            return -1;
> +        }
> +
> +        /* ensure that the secret is a NULL-terminated string */
> +        password = g_strndup((char*)secret, secretlen);

Both 'secret' and 'password' should be disposed of using
virSecureErase(String).

> +
> +        /* for now, just report an error rather than passing the password in
> +         * cleartext on the commandline */
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Password not yet supported for nbdkit sources"));
> +        return -1;
> +    }
> +
> +    if (proc->source->ncookies > 0) {
> +        /* for now, just report an error rather than passing cookies in
> +         * cleartext on the commandline */
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Cookies not yet supported for nbdkit sources"));
> +        return -1;
> +    }
> +
> +    if (proc->source->sslverify == VIR_TRISTATE_BOOL_NO) {
> +        virCommandAddArgPair(cmd, "sslverify", "false");

[...]

> @@ -675,3 +837,96 @@ qemuNbdkitProcessFree(qemuNbdkitProcess *proc)
>      g_clear_object(&proc->caps);
>      g_free(proc);
>  }
> +
> +
> +int
> +qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
> +                       virDomainObj *vm,
> +                       virQEMUDriver *driver)
> +{
> +    g_autoptr(virCommand) cmd = NULL;
> +    int rc;
> +    int exitstatus = 0;
> +    int cmdret = 0;
> +    VIR_AUTOCLOSE errfd = -1;
> +    virTimeBackOffVar timebackoff;
> +    bool socketCreated = false;
> +
> +    if (!(cmd = qemuNbdkitProcessBuildCommand(proc)))
> +        return -1;
> +
> +    VIR_DEBUG("starting nbdkit process for %s", proc->source->nodestorage);
> +    virCommandSetErrorFD(cmd, &errfd);

Using virCommandSetErrorBuffer will probably be cleaner given the
reading needed in the error fd.

> +    virCommandSetPidFile(cmd, proc->pidfile);

Once you de-clutter the formatting of the allowed protocols:

Reviewed-by: Peter Krempa <pkrempa at redhat.com>