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@xxxxxxxxxx>
> ---
> 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;
