Re: [libvirt PATCH v8 10/37] qemu: add functions to start and stop nbdkit
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
Reviewed-by: Peter Krempa
---
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
Re: [libvirt PATCH v8 10/37] qemu: add functions to start and stop nbdkit
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
> > > Reviewed-by: Peter Krempa
> > > ---
> > > 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.
Re: [libvirt PATCH v8 10/37] qemu: add functions to start and stop nbdkit
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
Reviewed-by: Peter Krempa
---
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,
+ >seclookupdef,
+ secrettype,
+ ,
+ ) < 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 = >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",
+
Re: [libvirt PATCH v8 10/37] qemu: add functions to start and stop nbdkit
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
> Reviewed-by: Peter Krempa
> ---
> 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,
> + >seclookupdef,
> + secrettype,
> + ,
> + ) < 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 = >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 *
[libvirt PATCH v8 10/37] qemu: add functions to start and stop nbdkit
Add some helper functions to build a virCommand object and run the
nbdkit process for a given virStorageSource.
Signed-off-by: Jonathon Jongsma
Reviewed-by: Peter Krempa
---
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,
+ >seclookupdef,
+ secrettype,
+ ,
+ ) < 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 = >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",
+
