3:08 p.m.
Richard W.M. Jones wrote:
From: Richard Jones <rjones(a)trick.home.annexia.org>
This reimplements parts of commit da0a4f8d1f6ddd302ceba028d87c6e009589e503
in a different, but compatible way.
We pass guestfs_vmchannel=tcp:<ip>:<port> on the command line. This
is intended to be used as follows (now and in future versions):
tcp:10.0.2.4:6666 for guestfwd vmchannel
tcp:10.0.2.2:<port> for future "no vmchannel" implementation
/dev/vcon4 for future virtio-console vmchannel*
It also accepts the old-style guestfs=10.0.2.4:6666 parameter which
is sent by older libraries, and turns this transparently into the
correct format above.
If no guestfs_vmchannel is passed, then this defaults to the guestfwd
vmchannel which older libraries would expect.
* Maybe this last one should be dev:/dev/vcon4 or file:/dev/vcon4, but
we don't need to decide that now.
---
daemon/guestfsd.c | 147 +++++++++++++++++++++++++++++++++++++++++++---------
src/guestfs.c | 4 ++
2 files changed, 125 insertions(+), 26 deletions(-)
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index a38452c..ce14dbf 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -68,8 +68,9 @@ int sysroot_len = 8;
int
main (int argc, char *argv[])
{
- static const char *options = "f?";
+ static const char *options = "fc:?";
static const struct option long_options[] = {
+ { "channel", 0, 0, 'c' },
Shouldn't this be:
{ "channel", required_argument, 0, 'c' },
{ "foreground", 0, 0, 'f' },
{ "help", 0, 0, '?' },
{ 0, 0, 0, 0 }
...
+ *
+ * Sources:
+ * --channel/-c option on the command line
+ * guestfs_vmchannel=... from the kernel command line
+ * guestfs=... from the kernel command line
+ * built-in default
+ *
+ * At the moment we expect this to contain "tcp:ip:port" but in
+ * future it might contain a device name, eg. "/dev/vcon4" for
+ * virtio-console vmchannel.
+ */
+ if (vmchannel == NULL && cmdline) {
+ char *p;
+ int len;
size_t len;
+ p = strstr (cmdline, "guestfs_vmchannel=");
+ if (p) {
+ len = strcspn (p + 18, " \t\n");
+ vmchannel = strndup (p + 18, len);
+ if (!vmchannel) {
+ perror ("strndup");
+ exit (1);
+ }
+ }
+
+ /* Old libraries passed guestfs=host:port. Rewrite it as tcp:host:port. */
+ if (vmchannel == NULL) {
+ p = strstr (cmdline, "guestfs=");
+ if (p) {
+ len = strcspn (p + 4, " \t\n");
+ vmchannel = strndup (p + 4, len);
+ if (!vmchannel) {
+ perror ("strndup");
+ exit (1);
+ }
+ memcpy (vmchannel, "tcp:", 4);
+ }
It took me a while to realize that the first two '4's above
stand for strlen("guestfs=") - strlen("tcp:')
Hoping to make it easier to follow, I wrote this:
if (vmchannel == NULL) {
p = strstr (cmdline, "guestfs=");
if (p) {
p += 8;
len = strcspn (p, " \t\n");
vmchannel = malloc (strlen("tcp:") + len + 1);
if (!vmchannel) {
perror ("malloc");
exit (1);
}
stpcpy (stpncpy (stpcpy (vmchannel, "tcp:"), p, len), "");
}
I don't particularly like the literal 8
or the duplicated "tcp:" strings, nor am I even
sure which is the lesser of the evils ;-)
And of course, if you're not used to the nested stpcpy idiom,
that last line will look weird. Even if you are, it's probably not
obvious that the outer stpcpy+"" arg is to NUL-terminate the result.
So maybe just add a comment to yours.
exit (1);
}
diff --git a/src/guestfs.c b/src/guestfs.c
index 55732f9..ec7473e 100644
--- a/src/guestfs.c
+++ b/src/guestfs.c
@@ -984,6 +984,7 @@ guestfs__launch (guestfs_h *g)
if (r == 0) { /* Child (qemu). */
char buf[256];
+ const char *vmchannel = NULL;
/* Set up the full command line. Do this in the subprocess so we
* don't need to worry about cleaning up.
@@ -1045,6 +1046,7 @@ guestfs__launch (guestfs_h *g)
}
add_cmdline (g, "-net");
add_cmdline (g, "nic,model=" NET_IF ",vlan=0");
+ vmchannel = "guestfs_vmchannel=tcp:" GUESTFWD_ADDR ":"
GUESTFWD_PORT " ";
#define LINUX_CMDLINE \
"panic=1 " /* force kernel to panic if daemon exits */ \
@@ -1058,9 +1060,11 @@ guestfs__launch (guestfs_h *g)
snprintf (buf, sizeof buf,
LINUX_CMDLINE
"%s" /* (selinux) */
+ "%s" /* (vmchannel) */
"%s" /* (verbose) */
"%s", /* (append) */
g->selinux ? "selinux=1 enforcing=0 " : "selinux=0
",
+ vmchannel ? vmchannel : "",
My first reaction was "What? No trailing space?"
Then I looked up to the definition of vmchannel, and
saw that it does indeed always ensure that it ends in
a trailing space.
IMHO, it'd be safer to build that trailing space into the format string,
even if it means sometimes having an extra space or two in the final
command line.
g->verbose ? "guestfs_verbose=1 " :
"",
g->append ? g->append : "");
[barely worth mentioning, I suppose, but...]
If g->append can push the total length beyond sizeof buf,
then it'd be nice to fail immediately, rather than using
the truncated line.