Re: [libvirt PATCH v8 10/37] qemu: add functions to start and stop nbdkit

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/20/23 7:24 AM, Pavel Hrdina wrote:
On Thu, Aug 31, 2023 at 04:39:50PM -0500, 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@xxxxxxxxxx>
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
---
  src/qemu/qemu_nbdkit.c | 250 +++++++++++++++++++++++++++++++++++++++++
  src/qemu/qemu_nbdkit.h |  10 ++
  2 files changed, 260 insertions(+)

diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index 9a2a89224d..6bf962d0f1 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -24,6 +24,8 @@
  #include "virerror.h"
  #include "virlog.h"
  #include "virpidfile.h"
+#include "virsecureerase.h"
+#include "virtime.h"
  #include "virutil.h"
  #include "qemu_block.h"
  #include "qemu_conf.h"
@@ -666,6 +668,168 @@ 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 */
+        virCommandAddArgPair(cmd, "protocols", "http,https");
+    } 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 %1$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);
+        virSecureErase(secret, secretlen);
+
+        /* 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"));
+
+        virSecureEraseString(password);
+
+        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");
+    }
+
+    if (proc->source->timeout > 0) {
+        g_autofree char *timeout = g_strdup_printf("%llu", proc->source->timeout);
+        virCommandAddArgPair(cmd, "timeout", timeout);
+    }
+
+    return 0;
+}
+
+
+static int
+qemuNbdkitProcessBuildCommandSSH(qemuNbdkitProcess *proc,
+                                 virCommand *cmd)
+{
+    const char *user = NULL;
+    virStorageNetHostDef *host = &proc->source->hosts[0];
+    g_autofree char *portstr = g_strdup_printf("%u", host->port);
+
+    /* nbdkit plugin name */
+    virCommandAddArg(cmd, "ssh");
+
+    virCommandAddArgPair(cmd, "host", host->name);
+    virCommandAddArgPair(cmd, "port", portstr);
+    virCommandAddArgPair(cmd, "path", proc->source->path);
+
+    if (proc->source->auth)
+        user = proc->source->auth->username;
+    else if (proc->source->ssh_user)
+        user = proc->source->ssh_user;
+
+    if (user)
+        virCommandAddArgPair(cmd, "user", user);
+
+    if (proc->source->ssh_host_key_check_disabled)
+        virCommandAddArgPair(cmd, "verify-remote-host", "false");
+
+    return 0;
+}
+
+
+static virCommand *
+qemuNbdkitProcessBuildCommand(qemuNbdkitProcess *proc)
+{
+    g_autoptr(virCommand) cmd = virCommandNewArgList(proc->caps->path,
+                                                     "--unix",
+                                                     proc->socketfile,
+                                                     "--foreground",
+                                                     NULL);
+
+    if (proc->source->readonly)
+        virCommandAddArg(cmd, "--readonly");
+
+    if (qemuNbdkitCapsGet(proc->caps, QEMU_NBDKIT_CAPS_FILTER_READAHEAD) &&
+        proc->source->readahead > 0)
+        virCommandAddArgPair(cmd, "--filter", "readahead");
+
+    switch (proc->source->protocol) {
+        case VIR_STORAGE_NET_PROTOCOL_HTTP:
+        case VIR_STORAGE_NET_PROTOCOL_HTTPS:
+        case VIR_STORAGE_NET_PROTOCOL_FTP:
+        case VIR_STORAGE_NET_PROTOCOL_FTPS:
+        case VIR_STORAGE_NET_PROTOCOL_TFTP:
+            if (qemuNbdkitProcessBuildCommandCurl(proc, cmd) < 0)
+                return NULL;
+            break;
+        case VIR_STORAGE_NET_PROTOCOL_SSH:
+            if (qemuNbdkitProcessBuildCommandSSH(proc, cmd) < 0)
+                return NULL;
+            break;
+
+        case VIR_STORAGE_NET_PROTOCOL_NONE:
+        case VIR_STORAGE_NET_PROTOCOL_NBD:
+        case VIR_STORAGE_NET_PROTOCOL_RBD:
+        case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
+        case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
+        case VIR_STORAGE_NET_PROTOCOL_ISCSI:
+        case VIR_STORAGE_NET_PROTOCOL_VXHS:
+        case VIR_STORAGE_NET_PROTOCOL_NFS:
+        case VIR_STORAGE_NET_PROTOCOL_LAST:
+            virReportError(VIR_ERR_NO_SUPPORT,
+                           _("protocol '%1$s' is not supported by nbdkit"),
+                           virStorageNetProtocolTypeToString(proc->source->protocol));
+            return NULL;
+    }
+
+    virCommandDaemonize(cmd);
+
+    return g_steal_pointer(&cmd);
+}
+
+
  void
  qemuNbdkitProcessFree(qemuNbdkitProcess *proc)
  {
@@ -674,3 +838,89 @@ 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;
+    g_autofree char *errbuf = NULL;
+    virTimeBackOffVar timebackoff;
+    g_autoptr(virURI) uri = NULL;
+    g_autofree char *uristring = NULL;
+
+    if (!(cmd = qemuNbdkitProcessBuildCommand(proc)))
+        return -1;
+
+    VIR_DEBUG("starting nbdkit process for %s", proc->source->nodestorage);
+    virCommandSetErrorBuffer(cmd, &errbuf);
+    virCommandSetPidFile(cmd, proc->pidfile);
+
+    if (qemuExtDeviceLogCommand(driver, vm, cmd, "nbdkit") < 0)
+        goto error;
+
+    if (qemuSecurityCommandRun(driver, vm, cmd, proc->user, proc->group, true, &exitstatus) < 0)
+        goto error;
+
+    if (exitstatus != 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("Could not start 'nbdkit'. exitstatus: %1$d"), exitstatus);
+        goto error;
+    }
+
+    if ((rc = virPidFileReadPath(proc->pidfile, &proc->pid)) < 0) {
+        virReportSystemError(-rc,
+                             _("Failed to read pidfile %1$s"),
+                             proc->pidfile);
+        goto error;
+    }
+
+    if (virTimeBackOffStart(&timebackoff, 1, 1000) < 0)
+        goto error;
+
+    while (virTimeBackOffWait(&timebackoff)) {
+        if (virFileExists(proc->socketfile))
+            return 0;
+
+        if (virProcessKill(proc->pid, 0) == 0)
+            continue;
+
+        VIR_WARN("nbdkit died unexpectedly");
+        goto errorlog;
+    }
+
+    VIR_WARN("nbdkit socket did not show up");
+
+ errorlog:
+    if ((uri = qemuBlockStorageSourceGetURI(proc->source)))
+        uristring = virURIFormat(uri);
+
+    virReportError(VIR_ERR_OPERATION_FAILED,
+                   _("Failed to connect to nbdkit for '%1$s': %2$s"),
+                   NULLSTR(uristring), NULLSTR(errbuf));
+
+ error:
+    qemuNbdkitProcessStop(proc);
+    return -1;
+}
+
+
+int
+qemuNbdkitProcessStop(qemuNbdkitProcess *proc)
+{
+    if (proc->pid < 0)
+        return 0;
+
+    VIR_DEBUG("Stopping nbdkit process %i", proc->pid);
+    virProcessKill(proc->pid, SIGTERM);

Coverity complains here that the return value of virProcessKill() is not
checked which leads me to a question if we should use
virProcessKillPainfully() instead.

With the code that is pushed the function qemuNbdkitProcessStop() is
called only within the qemu_nbdkit.c for these cases:

     - in qemuNbdkitProcessRestart() before starting the process again
       where we do not check if the original process was killed correctly
       or not,

     - in qemuNbdkitStopStorageSource() where we check return value of
       qemuNbdkitProcessStop() but it will always be 0,

     - in qemuNbdkitProcessStart() as error path where we don't check any
       return value.

To me it seems that the return value qemuNbdkitProcessStop can be changed
to void as we always return 0 and use virProcessKillPainfully() or
properly pass return value of virProcessKill() and check it for every
use of qemuNbdkitProcessStop().

Pavel

Good question. In one of my earlier series I had actually used virProcessKillPainfully(), but changed it based on a suggestion from Peter that it would be bad to kill it painfully if nbdkit was ever used in read-write mode. But apparently I forgot to handle a shutdown failure. An alternative would be to simply refuse to use nbdkit if the user requests read-write mode.

Jonathon




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux