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>
---
src/qemu/qemu_nbdkit.c | 251 +++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_nbdkit.h | 10 ++
2 files changed, 261 insertions(+)
diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index 291f988c7a..c5b0762f8d 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -26,6 +26,7 @@
#include "virerror.h"
#include "virlog.h"
#include "virpidfile.h"
+#include "virtime.h"
#include "virutil.h"
#include "qemu_block.h"
#include "qemu_conf.h"
@@ -636,6 +637,163 @@ 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");
+ 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;
+
+ 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,
+ &proc->source->auth->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);
+
+ /* 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");
+ }
+
+ 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,
+ "--exit-with-parent",
+ "--unix",
+ proc->socketfile,
+ "--foreground",
+ //"--selinux-label",
+ //selinux_label,
+ 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 '%s' is not supported by nbdkit"),
+ virStorageNetProtocolTypeToString(proc->source->protocol));
+ return NULL;
+ }
+
+ virCommandDaemonize(cmd);
+
+ return g_steal_pointer(&cmd);
+}
+
+
void
qemuNbdkitProcessFree(qemuNbdkitProcess *proc)
{
@@ -644,3 +802,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);
+ virCommandSetPidFile(cmd, proc->pidfile);
+
+ if (qemuExtDeviceLogCommand(driver, vm, cmd, "nbdkit") < 0)
+ goto error;
+
+ if (qemuSecurityCommandRun(driver, vm, cmd, proc->user, proc->group, &exitstatus, &cmdret) < 0)
+ goto error;
+
+ if (cmdret < 0 || exitstatus != 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not start 'nbdkit'. exitstatus: %d"), exitstatus);
+ goto error;
+ }
+
+ if ((rc = virPidFileReadPath(proc->pidfile, &proc->pid)) < 0) {
+ virReportSystemError(-rc,
+ _("Failed to read pidfile %s"),
+ proc->pidfile);
+ goto error;
+ }
+
+ if (virTimeBackOffStart(&timebackoff, 1, 1000) < 0)
+ goto error;
+
+ while (virTimeBackOffWait(&timebackoff)) {
+ if ((socketCreated = virFileExists(proc->socketfile)))
+ break;
+
+ if (virProcessKill(proc->pid, 0) == 0)
+ continue;
+
+ goto error;
+ }
+
+ if (!socketCreated) {
+ virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s",
+ _("nbdkit socket did not show up"));
+ goto error;
+ }
+
+ return 0;
+
+ error:
+ if (errfd > 0) {
+ g_autofree char *errbuf = g_new0(char, 1024);
+ if (read(errfd, errbuf, 1024) > 0)
+ virReportError(VIR_ERR_OPERATION_FAILED,
+ _("nbdkit failed to start and reported: %s"), errbuf);
+ }
+ qemuNbdkitProcessStop(proc);
+ return -1;
+}
+
+
+int
+qemuNbdkitProcessStop(qemuNbdkitProcess *proc)
+{
+ int ret;
+
+ if (proc->pid < 0)
+ return 0;
+
+ VIR_DEBUG("Stopping nbdkit process %i", proc->pid);
+ unlink(proc->pidfile);
+ unlink(proc->socketfile);
+
+ ret = virProcessKillPainfully(proc->pid, true);
+
+ if (ret >= 0)
+ proc->pid = -1;
+
+ return ret;
+}
diff --git a/src/qemu/qemu_nbdkit.h b/src/qemu/qemu_nbdkit.h
index 5cb7b0f21b..30cab744b0 100644
--- a/src/qemu/qemu_nbdkit.h
+++ b/src/qemu/qemu_nbdkit.h
@@ -40,6 +40,8 @@ typedef enum {
VIR_ENUM_DECL(qemuNbdkitCaps);
+typedef struct _virQEMUDriver virQEMUDriver;
+
qemuNbdkitCaps *
qemuNbdkitCapsNew(const char *path);
@@ -76,6 +78,14 @@ struct _qemuNbdkitProcess {
pid_t pid;
};
+int
+qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
+ virDomainObj *vm,
+ virQEMUDriver *driver);
+
+int
+qemuNbdkitProcessStop(qemuNbdkitProcess *proc);
+
void
qemuNbdkitProcessFree(qemuNbdkitProcess *proc);
--
2.37.3On Thu, Oct 20, 2022 at 16:59:00 -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>
> ---
> src/qemu/qemu_nbdkit.c | 251 +++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_nbdkit.h | 10 ++
> 2 files changed, 261 insertions(+)
[...]
> +static virCommand *
> +qemuNbdkitProcessBuildCommand(qemuNbdkitProcess *proc)
> +{
> + g_autoptr(virCommand) cmd = virCommandNewArgList(proc->caps->path,
> + "--exit-with-parent",
The documentation for this option states:
If the parent process exits, we exit. This can be used to avoid
complicated cleanup or orphaned nbdkit processes. There are some
important caveats with this, see "EXIT WITH PARENT" in
nbdkit-captive(1).
But this doesn't really make much sense in our case. We very
specifically daemonize the process so it becomes parent of init, thus
there's no point waiting for the parent.
> + "--unix",
> + proc->socketfile,
> + "--foreground",
> + //"--selinux-label",
> + //selinux_label,
> + NULL);
[...]
> +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);
> + virCommandSetPidFile(cmd, proc->pidfile);
> +
> + if (qemuExtDeviceLogCommand(driver, vm, cmd, "nbdkit") < 0)
> + goto error;
> +
> + if (qemuSecurityCommandRun(driver, vm, cmd, proc->user, proc->group, &exitstatus, &cmdret) < 0)
> + goto error;
> +
> + if (cmdret < 0 || exitstatus != 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not start 'nbdkit'. exitstatus: %d"), exitstatus);
> + goto error;
> + }
> +
> + if ((rc = virPidFileReadPath(proc->pidfile, &proc->pid)) < 0) {
> + virReportSystemError(-rc,
> + _("Failed to read pidfile %s"),
> + proc->pidfile);
> + goto error;
> + }
> +
> + if (virTimeBackOffStart(&timebackoff, 1, 1000) < 0)
> + goto error;
> +
> + while (virTimeBackOffWait(&timebackoff)) {
> + if ((socketCreated = virFileExists(proc->socketfile)))
> + break;
> +
> + if (virProcessKill(proc->pid, 0) == 0)
> + continue;
> +
> + goto error;
> + }
> +
> + if (!socketCreated) {
> + virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s",
> + _("nbdkit socket did not show up"));
> + goto error;
> + }
> +
> + return 0;
> +
> + error:
> + if (errfd > 0) {
> + g_autofree char *errbuf = g_new0(char, 1024);
> + if (read(errfd, errbuf, 1024) > 0)
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("nbdkit failed to start and reported: %s"), errbuf);
This error reporting doesn't seem to work. I tried starting a VM and got
an error from qemu that the NBD socket is not available:
error: internal error: process exited while connecting to monitor: 2022-12-09T15:12:25.558050Z qemu-system-x86_64: -blockdev {"driver":"nbd","server":{"type":"unix","path":"/var/lib/libvirt/qemu/domain-11-cd/nbdkit-libvirt-1-storage.socket"},"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}: Requested export not available
So I enabled logging from nbdkit into logging into the system journal
and got:
Dec 09 16:07:55 speedmetal nbdkit[2115190]: curl[1]: problem doing HEAD request to fetch size of URL [http://HOST:80/name]: Unsupported protocol: Protocol "https" not supported or disabled in libcurl
Which is true, the tested host forces https, but libvirt didn't report
it at all.
I think you'll need to convert this to properly capture the stdout/err
via virtlogd as we do with other external process helpers.
On 12/9/22 9:21 AM, Peter Krempa wrote:
> On Thu, Oct 20, 2022 at 16:59:00 -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>
>> ---
>> src/qemu/qemu_nbdkit.c | 251 +++++++++++++++++++++++++++++++++++++++++
>> src/qemu/qemu_nbdkit.h | 10 ++
>> 2 files changed, 261 insertions(+)
>
> [...]
>
>> +static virCommand *
>> +qemuNbdkitProcessBuildCommand(qemuNbdkitProcess *proc)
>> +{
>> + g_autoptr(virCommand) cmd = virCommandNewArgList(proc->caps->path,
>> + "--exit-with-parent",
>
> The documentation for this option states:
>
> If the parent process exits, we exit. This can be used to avoid
> complicated cleanup or orphaned nbdkit processes. There are some
> important caveats with this, see "EXIT WITH PARENT" in
> nbdkit-captive(1).
>
> But this doesn't really make much sense in our case. We very
> specifically daemonize the process so it becomes parent of init, thus
> there's no point waiting for the parent.
>
>
>> + "--unix",
>> + proc->socketfile,
>> + "--foreground",
>> + //"--selinux-label",
>> + //selinux_label,
>> + NULL);
>
> [...]
>
>> +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);
>> + virCommandSetPidFile(cmd, proc->pidfile);
>> +
>> + if (qemuExtDeviceLogCommand(driver, vm, cmd, "nbdkit") < 0)
>> + goto error;
>> +
>> + if (qemuSecurityCommandRun(driver, vm, cmd, proc->user, proc->group, &exitstatus, &cmdret) < 0)
>> + goto error;
>> +
>> + if (cmdret < 0 || exitstatus != 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Could not start 'nbdkit'. exitstatus: %d"), exitstatus);
>> + goto error;
>> + }
>> +
>> + if ((rc = virPidFileReadPath(proc->pidfile, &proc->pid)) < 0) {
>> + virReportSystemError(-rc,
>> + _("Failed to read pidfile %s"),
>> + proc->pidfile);
>> + goto error;
>> + }
>> +
>> + if (virTimeBackOffStart(&timebackoff, 1, 1000) < 0)
>> + goto error;
>> +
>> + while (virTimeBackOffWait(&timebackoff)) {
>> + if ((socketCreated = virFileExists(proc->socketfile)))
>> + break;
>> +
>> + if (virProcessKill(proc->pid, 0) == 0)
>> + continue;
>> +
>> + goto error;
>> + }
>> +
>> + if (!socketCreated) {
>> + virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s",
>> + _("nbdkit socket did not show up"));
>> + goto error;
>> + }
>> +
>> + return 0;
>> +
>> + error:
>> + if (errfd > 0) {
>> + g_autofree char *errbuf = g_new0(char, 1024);
>> + if (read(errfd, errbuf, 1024) > 0)
>> + virReportError(VIR_ERR_OPERATION_FAILED,
>> + _("nbdkit failed to start and reported: %s"), errbuf);
>
> This error reporting doesn't seem to work. I tried starting a VM and got
> an error from qemu that the NBD socket is not available:
>
> error: internal error: process exited while connecting to monitor: 2022-12-09T15:12:25.558050Z qemu-system-x86_64: -blockdev {"driver":"nbd","server":{"type":"unix","path":"/var/lib/libvirt/qemu/domain-11-cd/nbdkit-libvirt-1-storage.socket"},"node-name":"libvirt-1-storage","auto-read-only":true,"discard":"unmap"}: Requested export not available
>
> So I enabled logging from nbdkit into logging into the system journal
> and got:
>
> Dec 09 16:07:55 speedmetal nbdkit[2115190]: curl[1]: problem doing HEAD request to fetch size of URL [http://HOST:80/name]: Unsupported protocol: Protocol "https" not supported or disabled in libcurl
>
> Which is true, the tested host forces https, but libvirt didn't report
> it at all.
>
> I think you'll need to convert this to properly capture the stdout/err
> via virtlogd as we do with other external process helpers.
>
Adding Rich to cc on this subthread as well.
Peter and I were discussing this on IRC and were wondering if nbdkit
provides (or could provide) something that could help with error
reporting here. The issue is that if you configure the the network
source incorrectly, nbdkit will start up fine, and won't report an error
until qemu connects to the unix socket and tries to access the nbd
export. At that point, we report the unhelpful error Peter mentions
above ("Requested export not available") rather than something helpful
(e.g. "Couldn't resolve host name: site-that-doesnt-exist.com").
One possibility that we discussed was that libvirt could link against
libnbd and try to open the socket ourselves before attempting to pass it
to qemu. This would theoretically trigger an error from nbdkit and allow
us to report that error before we even get to the point of starting
qemu. That would at least help catch misconfigurations. But it seems
unfortunate to add a new dependency just for that. If we could simply
tell nbdkit to try to access the remote host and fail early rather than
waiting for a nbd client, that would be nicer from my point of view.
Any thoughts? Based on my past experience, I wouldn't be surprised if
you've already though of this and there's a plugin that already provides
this functionality ;)
Jonathon
On Thu, Oct 20, 2022 at 16:59:00 -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>
> ---
> src/qemu/qemu_nbdkit.c | 251 +++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_nbdkit.h | 10 ++
> 2 files changed, 261 insertions(+)
>
> diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
> index 291f988c7a..c5b0762f8d 100644
> --- a/src/qemu/qemu_nbdkit.c
> +++ b/src/qemu/qemu_nbdkit.c
> @@ -26,6 +26,7 @@
> #include "virerror.h"
> #include "virlog.h"
> #include "virpidfile.h"
> +#include "virtime.h"
> #include "virutil.h"
> #include "qemu_block.h"
> #include "qemu_conf.h"
> @@ -636,6 +637,163 @@ 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");
> + 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;
> +
> + 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,
> + &proc->source->auth->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);
> +
> + /* 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");
Clever way to bypass the syntaxcheck mandating translations ... but we
should probably fix the check to not allow this in the future.
> + 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;
So there's a bit problem with this, which also explains my reluctance to
simply supporting the SSH protocol in the current state without properly
designing other required parameters for SSH authentication. And even
then it will most likely break "historical compatibility".
So historically we didn't implement ssh protocol at all. Users (notably
libguestfs) worked this around by adding a wrapper QCOW2 image and
setting up the backing store string such that qemu would access the ssh
image.
When blockdev support was added I needed a way to port the existing
functionality specifically for libguestfs. This made me add the ssh
protocol and forwart the minimum set of properties to the image (thus
the 'ssh_user' field.
But there's another thing that was always configured out-of-band (via
environment variables) and that is the ssh agent socket path or ssh key
path.
These are not present in the virStorageSource because they are not even
configured for the blockdev 'ssh' protocol driver directly.
Now with switching to nbdkit this means agent/sshkey authentication will
necessarily break for such usage as we simply don't have the
information.
So what to do about it?
We can either break the old way completely, which I guess would be
mostly okay since libguestfs most likely now uses nbdkit anyways, and
then design it properly. Obviously since the qemu ssh blockdev backend
driver still configures stuff using environment variables thus the new
configuration will be available only when nbdkit is in use.
Other possibility, if we want to keep old functionality working as it
was, is to avoid the use of nbdkit.
Either way the implementation of SSH as done in this series simply just
breaks SSH usage completely, by not being able to support the old hacky
way of using agent/sshkey, not implementing any new support and not even
implementing password authentication.
What now? I don't really care too deeply about the hacky impl we have
now. If libguestfs is no longer using I reckon we can simply break it
and not deal with the fallout.
I'm not sure how others care about that though.
Obviously another option (besides doing a proper desing on
authentication config) is to not touch anything ssh-related for now.
> +
> + 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,
> + "--exit-with-parent",
> + "--unix",
> + proc->socketfile,
> + "--foreground",
> + //"--selinux-label",
> + //selinux_label,
Leftovers?
> + NULL);
> +
> + if (proc->source->readonly)
> + virCommandAddArg(cmd, "--readonly");
Note that when you want to do blockjobs qemu is currently re-opening the
image in read-write mode if necessary. With the external process this
won't be possible.
Luckily though IIRC only ssh had write support and since we never
supported SSH as the top layer image it's very unlikely that anybody
ever attempted blockjobs over SSH.
> +
> + 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 '%s' is not supported by nbdkit"),
> + virStorageNetProtocolTypeToString(proc->source->protocol));
> + return NULL;
> + }
> +
> + virCommandDaemonize(cmd);
> +
> + return g_steal_pointer(&cmd);
> +}
> +
> +
> void
> qemuNbdkitProcessFree(qemuNbdkitProcess *proc)
> {
> @@ -644,3 +802,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);
> + virCommandSetPidFile(cmd, proc->pidfile);
> +
> + if (qemuExtDeviceLogCommand(driver, vm, cmd, "nbdkit") < 0)
> + goto error;
> +
> + if (qemuSecurityCommandRun(driver, vm, cmd, proc->user, proc->group, &exitstatus, &cmdret) < 0)
> + goto error;
> +
> + if (cmdret < 0 || exitstatus != 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not start 'nbdkit'. exitstatus: %d"), exitstatus);
> + goto error;
> + }
> +
> + if ((rc = virPidFileReadPath(proc->pidfile, &proc->pid)) < 0) {
> + virReportSystemError(-rc,
> + _("Failed to read pidfile %s"),
> + proc->pidfile);
> + goto error;
> + }
> +
> + if (virTimeBackOffStart(&timebackoff, 1, 1000) < 0)
> + goto error;
> +
> + while (virTimeBackOffWait(&timebackoff)) {
> + if ((socketCreated = virFileExists(proc->socketfile)))
Can't we pass a pre-opened FD to the socket to NBDkit so that we don't
have to wait for it here?
> + break;
> +
> + if (virProcessKill(proc->pid, 0) == 0)
> + continue;
> +
> + goto error;
> + }
> +
> + if (!socketCreated) {
> + virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s",
> + _("nbdkit socket did not show up"));
> + goto error;
> + }
> +
> + return 0;
> +
> + error:
> + if (errfd > 0) {
> + g_autofree char *errbuf = g_new0(char, 1024);
> + if (read(errfd, errbuf, 1024) > 0)
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + _("nbdkit failed to start and reported: %s"), errbuf);
> + }
> + qemuNbdkitProcessStop(proc);
> + return -1;
> +}
> +
> +
> +int
> +qemuNbdkitProcessStop(qemuNbdkitProcess *proc)
> +{
> + int ret;
> +
> + if (proc->pid < 0)
> + return 0;
> +
> + VIR_DEBUG("Stopping nbdkit process %i", proc->pid);
> + unlink(proc->pidfile);
> + unlink(proc->socketfile);
> +
> + ret = virProcessKillPainfully(proc->pid, true);
So I presume that the intention is to never use this in read-write mode.
Otherwise I'd expect graceful shutdown.
You probably want an explicit check that nbdkit will never be used in
read-write mode if you want to kill it this way.
> +
> + if (ret >= 0)
> + proc->pid = -1;
> +
> + return ret;
On 12/9/22 4:09 AM, Peter Krempa wrote:
> On Thu, Oct 20, 2022 at 16:59:00 -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>
>> ---
>> src/qemu/qemu_nbdkit.c | 251 +++++++++++++++++++++++++++++++++++++++++
>> src/qemu/qemu_nbdkit.h | 10 ++
>> 2 files changed, 261 insertions(+)
>>
...
>> +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;
>
> So there's a bit problem with this, which also explains my reluctance to
> simply supporting the SSH protocol in the current state without properly
> designing other required parameters for SSH authentication. And even
> then it will most likely break "historical compatibility".
>
> So historically we didn't implement ssh protocol at all. Users (notably
> libguestfs) worked this around by adding a wrapper QCOW2 image and
> setting up the backing store string such that qemu would access the ssh
> image.
>
> When blockdev support was added I needed a way to port the existing
> functionality specifically for libguestfs. This made me add the ssh
> protocol and forwart the minimum set of properties to the image (thus
> the 'ssh_user' field.
>
> But there's another thing that was always configured out-of-band (via
> environment variables) and that is the ssh agent socket path or ssh key
> path.
>
> These are not present in the virStorageSource because they are not even
> configured for the blockdev 'ssh' protocol driver directly.
>
> Now with switching to nbdkit this means agent/sshkey authentication will
> necessarily break for such usage as we simply don't have the
> information.
>
> So what to do about it?
>
> We can either break the old way completely, which I guess would be
> mostly okay since libguestfs most likely now uses nbdkit anyways, and
> then design it properly. Obviously since the qemu ssh blockdev backend
> driver still configures stuff using environment variables thus the new
> configuration will be available only when nbdkit is in use.
>
> Other possibility, if we want to keep old functionality working as it
> was, is to avoid the use of nbdkit.
>
> Either way the implementation of SSH as done in this series simply just
> breaks SSH usage completely, by not being able to support the old hacky
> way of using agent/sshkey, not implementing any new support and not even
> implementing password authentication.
>
> What now? I don't really care too deeply about the hacky impl we have
> now. If libguestfs is no longer using I reckon we can simply break it
> and not deal with the fallout.
>
> I'm not sure how others care about that though.
>
> Obviously another option (besides doing a proper desing on
> authentication config) is to not touch anything ssh-related for now.
>
Adding Rich to cc in case he has anything to add to this discussion.
The problem, as I understand it, is the desire to remove the qemu block
plugins from distributions. If that happens, then avoiding the use of
nbdkit within libvirt won't solve anything -- the ssh use case will
still break due to the absence of these qemu plugins.
I'm afraid I don't have enough background information about when ssh is
used and how common it is to make these decisions on my own. But I agree
that the current implementation in this patch series is not complete
enough to be useful.
Jonathon
On Fri, Dec 09, 2022 at 11:17:22AM -0600, Jonathon Jongsma wrote:
> On 12/9/22 4:09 AM, Peter Krempa wrote:
> >On Thu, Oct 20, 2022 at 16:59:00 -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>
> >>---
> >> src/qemu/qemu_nbdkit.c | 251 +++++++++++++++++++++++++++++++++++++++++
> >> src/qemu/qemu_nbdkit.h | 10 ++
> >> 2 files changed, 261 insertions(+)
> >>
>
> ...
>
>
> >>+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;
> >
> >So there's a bit problem with this, which also explains my reluctance to
> >simply supporting the SSH protocol in the current state without properly
> >designing other required parameters for SSH authentication. And even
> >then it will most likely break "historical compatibility".
> >
> >So historically we didn't implement ssh protocol at all. Users (notably
> >libguestfs) worked this around by adding a wrapper QCOW2 image and
> >setting up the backing store string such that qemu would access the ssh
> >image.
> >
> >When blockdev support was added I needed a way to port the existing
> >functionality specifically for libguestfs. This made me add the ssh
> >protocol and forwart the minimum set of properties to the image (thus
> >the 'ssh_user' field.
Off topic, but I wanted to check what we do now. We generate this XML
fragment:
$ guestfish --format=raw -a ssh://foo/var/tmp/fedora-36.img -i -vx
...
<disk device="disk" type="network">
<source protocol="ssh" name="/var/tmp/fedora-36.img">
<host name="foo"/>
</source>
<target dev="sda" bus="scsi"/>
<driver name="qemu" type="raw" cache="writeback"/>
<address type="drive" controller="0" bus="0" target="0" unit="0"/>
</disk>
which I think is correct(?)
> >But there's another thing that was always configured out-of-band (via
> >environment variables) and that is the ssh agent socket path or ssh key
> >path.
Right - this doesn't work in fact:
Original error from libvirt: internal error: process exited while connecting to monitor: 2022-12-09T18:16:45.674960Z qemu-kvm: -blockdev {"driver":"ssh","path":"/var/tmp/fedora-36.img","server":{"host":"foo","port":"22"},"node-name":"libvirt-2-storage","cache":{"direct":false,"no-flush":false},"auto-read-only":true,"discard":"unmap"}: failed to authenticate using publickey authentication and the identities held by your ssh-agent [code=1 int1=-1]
Am I doing something wrong here? I have to confess it's been a very
long time since I tried to use the ssh support directly in libguestfs
(see below for why). So I don't quite remember how it's supposed to
work, or else it broke and I didn't notice.
> >These are not present in the virStorageSource because they are not even
> >configured for the blockdev 'ssh' protocol driver directly.
> >
> >Now with switching to nbdkit this means agent/sshkey authentication will
> >necessarily break for such usage as we simply don't have the
> >information.
> >
> >So what to do about it?
> >
> >We can either break the old way completely, which I guess would be
> >mostly okay since libguestfs most likely now uses nbdkit anyways, and
> >then design it properly. Obviously since the qemu ssh blockdev backend
> >driver still configures stuff using environment variables thus the new
> >configuration will be available only when nbdkit is in use.
So for libguestfs as above we're using libvirt (and hence indirectly
qemu's ssh). That's broken, but no one reported the bug so I guess
it's little used.
But you're right that for virt-v2v we switched to using
nbdkit-ssh-plugin a while back and that really is used and tested
extensively. (In this case we use libvirt + qemu's nbd driver to talk
to the nbdkit endpoint.)
> >Other possibility, if we want to keep old functionality working as it
> >was, is to avoid the use of nbdkit.
> >
> >Either way the implementation of SSH as done in this series simply just
> >breaks SSH usage completely, by not being able to support the old hacky
> >way of using agent/sshkey, not implementing any new support and not even
> >implementing password authentication.
> >
> >What now? I don't really care too deeply about the hacky impl we have
> >now. If libguestfs is no longer using I reckon we can simply break it
> >and not deal with the fallout.
> >
> >I'm not sure how others care about that though.
> >
> >Obviously another option (besides doing a proper desing on
> >authentication config) is to not touch anything ssh-related for now.
> >
>
> Adding Rich to cc in case he has anything to add to this discussion.
>
> The problem, as I understand it, is the desire to remove the qemu
> block plugins from distributions. If that happens, then avoiding the
> use of nbdkit within libvirt won't solve anything -- the ssh use
> case will still break due to the absence of these qemu plugins.
>
> I'm afraid I don't have enough background information about when ssh
> is used and how common it is to make these decisions on my own. But
> I agree that the current implementation in this patch series is not
> complete enough to be useful.
From the libguestfs point of view, we are generating the
<disk .. type="network"><source protocol="ssh"> fragment above
and hoping that libvirt's implementation does the right thing.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
On Fri, Dec 09, 2022 at 18:23:59 +0000, Richard W.M. Jones wrote:
> On Fri, Dec 09, 2022 at 11:17:22AM -0600, Jonathon Jongsma wrote:
> > On 12/9/22 4:09 AM, Peter Krempa wrote:
> > >On Thu, Oct 20, 2022 at 16:59:00 -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>
> > >>---
> > >> src/qemu/qemu_nbdkit.c | 251 +++++++++++++++++++++++++++++++++++++++++
> > >> src/qemu/qemu_nbdkit.h | 10 ++
> > >> 2 files changed, 261 insertions(+)
> > >>
> >
> > ...
> >
> >
> > >>+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;
> > >
> > >So there's a bit problem with this, which also explains my reluctance to
> > >simply supporting the SSH protocol in the current state without properly
> > >designing other required parameters for SSH authentication. And even
> > >then it will most likely break "historical compatibility".
> > >
> > >So historically we didn't implement ssh protocol at all. Users (notably
> > >libguestfs) worked this around by adding a wrapper QCOW2 image and
> > >setting up the backing store string such that qemu would access the ssh
> > >image.
> > >
> > >When blockdev support was added I needed a way to port the existing
> > >functionality specifically for libguestfs. This made me add the ssh
> > >protocol and forwart the minimum set of properties to the image (thus
> > >the 'ssh_user' field.
>
> Off topic, but I wanted to check what we do now. We generate this XML
> fragment:
>
> $ guestfish --format=raw -a ssh://foo/var/tmp/fedora-36.img -i -vx
>
> ...
>
> <disk device="disk" type="network">
> <source protocol="ssh" name="/var/tmp/fedora-36.img">
> <host name="foo"/>
> </source>
> <target dev="sda" bus="scsi"/>
> <driver name="qemu" type="raw" cache="writeback"/>
> <address type="drive" controller="0" bus="0" target="0" unit="0"/>
> </disk>
>
> which I think is correct(?)
Kind of ... it uses the un-documented way to instantiate SSH which is
undocumented due to the reasons I mentioned originally, but it should
work to the extent it used to in the past with qemu.
> > >But there's another thing that was always configured out-of-band (via
> > >environment variables) and that is the ssh agent socket path or ssh key
> > >path.
>
> Right - this doesn't work in fact:
>
> Original error from libvirt: internal error: process exited while connecting to monitor: 2022-12-09T18:16:45.674960Z qemu-kvm: -blockdev {"driver":"ssh","path":"/var/tmp/fedora-36.img","server":{"host":"foo","port":"22"},"node-name":"libvirt-2-storage","cache":{"direct":false,"no-flush":false},"auto-read-only":true,"discard":"unmap"}: failed to authenticate using publickey authentication and the identities held by your ssh-agent [code=1 int1=-1]
The error message you are getting here means that both agent and
privkey authentication failed.
> Am I doing something wrong here? I have to confess it's been a very
> long time since I tried to use the ssh support directly in libguestfs
> (see below for why). So I don't quite remember how it's supposed to
> work, or else it broke and I didn't notice.
In the configuration that guestfish/libguestfs uses it simply uses a SSH
disk and doesn't configure the 'SSH_AUTH_SOCK=' environment variable via
the 'qemu' namespace env pass options. libvirt will not pass it by
design so agent based authentication is out of the question.
Since the XML doesn't have any way to specify which auth to use the last
fallback option is to use the default ssh key/identity. Qemu also
doesn't have any option to unlock the ssh key so really the only option
is password-less keys.
If you have them configured it still works at least in my testing.
This obviously works only for 'session' mode connections, as in the
system/privileged mode qemu is spawned as a different user which doesn't
even have a home directory so there's no way to give it ssh keys.
This means that ssh-backed disks worked only:
1) in session mode, if you env-passed the agent, or used password-less
keys
2) in system mode, if you env-passed the agent (and hacked around
selinux somehow)
Based on the above, the only working option for now is session-mode with
password-less keys.
> > >These are not present in the virStorageSource because they are not even
> > >configured for the blockdev 'ssh' protocol driver directly.
> > >
> > >Now with switching to nbdkit this means agent/sshkey authentication will
> > >necessarily break for such usage as we simply don't have the
> > >information.
> > >
> > >So what to do about it?
> > >
> > >We can either break the old way completely, which I guess would be
> > >mostly okay since libguestfs most likely now uses nbdkit anyways, and
> > >then design it properly. Obviously since the qemu ssh blockdev backend
> > >driver still configures stuff using environment variables thus the new
> > >configuration will be available only when nbdkit is in use.
>
> So for libguestfs as above we're using libvirt (and hence indirectly
> qemu's ssh). That's broken, but no one reported the bug so I guess
> it's little used.
Or the users are using it with password-less keys as their only option.
> But you're right that for virt-v2v we switched to using
> nbdkit-ssh-plugin a while back and that really is used and tested
> extensively. (In this case we use libvirt + qemu's nbd driver to talk
> to the nbdkit endpoint.)
>
> > >Other possibility, if we want to keep old functionality working as it
> > >was, is to avoid the use of nbdkit.
> > >
> > >Either way the implementation of SSH as done in this series simply just
> > >breaks SSH usage completely, by not being able to support the old hacky
> > >way of using agent/sshkey, not implementing any new support and not even
> > >implementing password authentication.
> > >
> > >What now? I don't really care too deeply about the hacky impl we have
> > >now. If libguestfs is no longer using I reckon we can simply break it
> > >and not deal with the fallout.
> > >
> > >I'm not sure how others care about that though.
> > >
> > >Obviously another option (besides doing a proper desing on
> > >authentication config) is to not touch anything ssh-related for now.
> > >
> >
> > Adding Rich to cc in case he has anything to add to this discussion.
> >
> > The problem, as I understand it, is the desire to remove the qemu
> > block plugins from distributions. If that happens, then avoiding the
> > use of nbdkit within libvirt won't solve anything -- the ssh use
> > case will still break due to the absence of these qemu plugins.
> >
> > I'm afraid I don't have enough background information about when ssh
> > is used and how common it is to make these decisions on my own. But
> > I agree that the current implementation in this patch series is not
> > complete enough to be useful.
>
> From the libguestfs point of view, we are generating the
> <disk .. type="network"><source protocol="ssh"> fragment above
> and hoping that libvirt's implementation does the right thing.
Well it does in a very specific limited set of options which were
implemented as a hack to prevent breakage when we switched to blockdev
mode.
Now with these patches 'nbdkit' would most likely work the same way in
the exact scenario where session mode libvirt is used with password-less
keys.
What will break is if anybody used agent and passed it via the
environment because the nbdkit process will not get the environment
override which was specified for qemu. (And it should not get it either).
Obviously env pass is where we don't give strong non-breakage
guarantees.
Ideally we'll provide a better interface for setting authentication with
ssh which will allow you to pass specific keys, agent socket, or
password for password-based auth, but since libguestfs is
not actually passing the agent socket (any more?) and simply works only
with the default identity, which should work also with nbdkit.
In my opinion, since there's no known user of agent-based auth passed
via qemu:env, we can break it as it was never really supported.
On Fri, Dec 09, 2022 at 06:23:59PM +0000, Richard W.M. Jones wrote:
> $ guestfish --format=raw -a ssh://foo/var/tmp/fedora-36.img -i -vx
>
> ...
>
> <disk device="disk" type="network">
> <source protocol="ssh" name="/var/tmp/fedora-36.img">
> <host name="foo"/>
> </source>
> <target dev="sda" bus="scsi"/>
> <driver name="qemu" type="raw" cache="writeback"/>
> <address type="drive" controller="0" bus="0" target="0" unit="0"/>
> </disk>
Actually I remembered in the "read-only"[1] case we do create a qcow2
overlay:
$ guestfish --ro --format=raw -a ssh://foo/var/tmp/fedora-36.img -i
...
libguestfs: trace: disk_create "/tmp/libguestfszdXqqC/overlay1.qcow2" "qcow2" -1 "backingfile:ssh://foo/var/tmp/fedora-36.img" "backingformat:raw"
libguestfs: command: run: qemu-img
libguestfs: command: run: \ create
libguestfs: command: run: \ -f qcow2
libguestfs: command: run: \ -o backing_file=ssh://foo/var/tmp/fedora-36.img,backing_fmt=raw
libguestfs: command: run: \ /tmp/libguestfszdXqqC/overlay1.qcow2
Formatting '/tmp/libguestfszdXqqC/overlay1.qcow2', fmt=qcow2 cluster_size=65536 extended_l2=off compression_type=zlib size=6442450944 backing_file=ssh://foo/var/tmp/fedora-36.img backing_fmt=raw lazy_refcounts=off refcount_bits=16
...
<disk device="disk" type="file">
<source file="/tmp/libguestfszdXqqC/overlay1.qcow2"/>
<target dev="sda" bus="scsi"/>
<driver name="qemu" type="qcow2" cache="unsafe"/>
<address type="drive" controller="0" bus="0" target="0" unit="0"/>
</disk>
which might be what you were thinking about. Actually this doesn't
work either:
Original error from libvirt: internal error: process exited while connecting to monitor: 2022-12-09T20:17:41.074400Z qemu-kvm: -blockdev {"driver":"ssh","path":"var/tmp/fedora-36.img","server":{"host":"foo","port":"22"},"node-name":"libvirt-4-storage","cache":{"direct":false,"no-flush":true},"auto-read-only":true,"discard":"unmap"}: failed to authenticate using publickey authentication and the identities held by your ssh-agent [code=1 int1=-1]
Rich.
[1] Not really "read-only" - we must create a r/w overlay because
otherwise replaying journals in filesystems does not work. The
overlay is discarded afterwards.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines. Supports shell scripting,
bindings from many languages. http://libguestfs.org
© 2016 - 2026 Red Hat, Inc.