Add some helper functions to build a virCommand object and run the
nbdkit process for a given virStorageSource.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@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);
+
+ unlink(proc->pidfile);
+ unlink(proc->socketfile);
+ proc->pid = -1;
+
+ return 0;
+}
diff --git a/src/qemu/qemu_nbdkit.h b/src/qemu/qemu_nbdkit.h
index 8844bba13c..ccd418b7d3 100644
--- a/src/qemu/qemu_nbdkit.h
+++ b/src/qemu/qemu_nbdkit.h
@@ -38,6 +38,8 @@ typedef enum {
VIR_ENUM_DECL(qemuNbdkitCaps);
+typedef struct _virQEMUDriver virQEMUDriver;
+
qemuNbdkitCaps *
qemuNbdkitCapsNew(const char *path);
@@ -74,6 +76,14 @@ struct _qemuNbdkitProcess {
pid_t pid;
};
+int
+qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
+ virDomainObj *vm,
+ virQEMUDriver *driver);
+
+int
+qemuNbdkitProcessStop(qemuNbdkitProcess *proc);
+
void
qemuNbdkitProcessFree(qemuNbdkitProcess *proc);
--
2.41.0On 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@redhat.com>
> Reviewed-by: Peter Krempa <pkrempa@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
> +
> + unlink(proc->pidfile);
> + unlink(proc->socketfile);
> + proc->pid = -1;
> +
> + return 0;
> +}
> diff --git a/src/qemu/qemu_nbdkit.h b/src/qemu/qemu_nbdkit.h
> index 8844bba13c..ccd418b7d3 100644
> --- a/src/qemu/qemu_nbdkit.h
> +++ b/src/qemu/qemu_nbdkit.h
> @@ -38,6 +38,8 @@ typedef enum {
>
> VIR_ENUM_DECL(qemuNbdkitCaps);
>
> +typedef struct _virQEMUDriver virQEMUDriver;
> +
> qemuNbdkitCaps *
> qemuNbdkitCapsNew(const char *path);
>
> @@ -74,6 +76,14 @@ struct _qemuNbdkitProcess {
> pid_t pid;
> };
>
> +int
> +qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
> + virDomainObj *vm,
> + virQEMUDriver *driver);
> +
> +int
> +qemuNbdkitProcessStop(qemuNbdkitProcess *proc);
> +
> void
> qemuNbdkitProcessFree(qemuNbdkitProcess *proc);
>
> --
> 2.41.0
>
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@redhat.com>
>> Reviewed-by: Peter Krempa <pkrempa@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
On Thu, Sep 21, 2023 at 12:50:52 -0500, Jonathon Jongsma wrote:
> 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@redhat.com>
> > > Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> > > ---
> > > src/qemu/qemu_nbdkit.c | 250 +++++++++++++++++++++++++++++++++++++++++
> > > src/qemu/qemu_nbdkit.h | 10 ++
> > > 2 files changed, 260 insertions(+)
[...]
> > > + 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.
We can use the same algorithm as with the qemu process where we first
issue SIGTERM, thus if the process is responsive it can execute the
shutdown actions. Otherwise it'll get SIGKILL right after.
On 9/21/23 1:10 PM, Peter Krempa wrote:
> On Thu, Sep 21, 2023 at 12:50:52 -0500, Jonathon Jongsma wrote:
>> 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@redhat.com>
>>>> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
>>>> ---
>>>> src/qemu/qemu_nbdkit.c | 250 +++++++++++++++++++++++++++++++++++++++++
>>>> src/qemu/qemu_nbdkit.h | 10 ++
>>>> 2 files changed, 260 insertions(+)
>
> [...]
>
>>>> + 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.
>
> We can use the same algorithm as with the qemu process where we first
> issue SIGTERM, thus if the process is responsive it can execute the
> shutdown actions. Otherwise it'll get SIGKILL right after.
>
That sounds almost identical to what virProcessKillPainfully() does.
Jonathon
© 2016 - 2026 Red Hat, Inc.