12:50 p.m.
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(a)redhat.com>
> Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
> ---
> 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