diff mbox series

[v6,06/18] libxl: write qemu arguments into separate xenstore keys

Message ID 20200518011353.326287-7-jandryuk@gmail.com (mailing list archive)
State Superseded
Headers show
Series Add support for qemu-xen runnning in a Linux-based stubdomain | expand

Commit Message

Jason Andryuk May 18, 2020, 1:13 a.m. UTC 
From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

This allows using arguments with spaces, like -append, without
nominating any special "separator" character.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Re-work to use libxl_xs_* functions in a loop.  Also write arguments in
dm-argv directory instead of overloading mini-os's dmargs string.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
Changes in v3:
 - previous version of this patch "libxl: use \x1b to separate qemu
   arguments for linux stubdomain" used specific non-printable
   separator, but it was rejected as xenstore doesn't cope well with
   non-printable chars
Changes in v6:
 - Re-work to use libxl__xs_ functions in a loop.
 - Drop rtc/timeoffset
---
 tools/libxl/libxl_dm.c | 56 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

Comments

Ian Jackson May 18, 2020, 2:19 p.m. UTC | #1 
Jason Andryuk writes ("[PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys"):
> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
...
> +static int libxl__write_stub_linux_dm_argv(libxl__gc *gc,
> +                                           int dm_domid, int guest_domid,
> +                                           char **args)
> +{

Thanks for the changes.

> +    xs_transaction_t t = XBT_NULL;
...
> +    rc = libxl__xs_read_mandatory(gc, XBT_NULL,
> +                                  GCSPRINTF("/local/domain/%d/vm", guest_domid),
> +                                  &vm_path);
> +    if (rc)
> +        return rc;

I think this should be "goto out".  That conforms to standard libxl
error handling discipline and avoids future leak bugs etc.
libxl__xs_transaction_abort is a no-op with t==NULL.

Also, it is not clear to me why you chose to put this outside the
transaction loop.  Can you either put it inside the transaction loop,
or produce an explanation as to why this approach is race-free...

Thanks,
Ian.
Jason Andryuk May 18, 2020, 3:20 p.m. UTC | #2 
On Mon, May 18, 2020 at 10:20 AM Ian Jackson <ian.jackson@citrix.com> wrote:
>
> Jason Andryuk writes ("[PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys"):
> > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ...
> > +static int libxl__write_stub_linux_dm_argv(libxl__gc *gc,
> > +                                           int dm_domid, int guest_domid,
> > +                                           char **args)
> > +{
>
> Thanks for the changes.
>
> > +    xs_transaction_t t = XBT_NULL;
> ...
> > +    rc = libxl__xs_read_mandatory(gc, XBT_NULL,
> > +                                  GCSPRINTF("/local/domain/%d/vm", guest_domid),
> > +                                  &vm_path);
> > +    if (rc)
> > +        return rc;
>
> I think this should be "goto out".  That conforms to standard libxl
> error handling discipline and avoids future leak bugs etc.
> libxl__xs_transaction_abort is a no-op with t==NULL.
>
> Also, it is not clear to me why you chose to put this outside the
> transaction loop.  Can you either put it inside the transaction loop,
> or produce an explanation as to why this approach is race-free...

I just matched the old code's transaction only around the write.  "vm"
shouldn't change during runtime, but I can make the changes as you
suggest.

-Jason
Ian Jackson May 18, 2020, 4:34 p.m. UTC | #3 
Jason Andryuk writes ("Re: [PATCH v6 06/18] libxl: write qemu arguments into separate xenstore keys"):
> On Mon, May 18, 2020 at 10:20 AM Ian Jackson <ian.jackson@citrix.com> wrote:
> > I think this should be "goto out".  That conforms to standard libxl
> > error handling discipline and avoids future leak bugs etc.
> > libxl__xs_transaction_abort is a no-op with t==NULL.
> >
> > Also, it is not clear to me why you chose to put this outside the
> > transaction loop.  Can you either put it inside the transaction loop,
> > or produce an explanation as to why this approach is race-free...
> 
> I just matched the old code's transaction only around the write.  "vm"
> shouldn't change during runtime, but I can make the changes as you
> suggest.

Ah I see.  I hadn't spotted this duplication.

As there is only one caller of libxl__write_stub_dmargs the messing
about with %d/vm and the transaction and so on could be factored out
and only the actual arg processing made conditional.

I would prefer that, to be honest.  But I don't want to derail this
series at this point by asking you to take on refactorings that I
ought to have asked for sooner.

So I'll take it if you make the new code the way I like it, as I
suggest above.  Maybe it will be refactored later (perhaps even by
me...)

Regards,
Ian.
diff mbox series

Patch

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index dc1717bc12..eaed6e8ee7 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2066,6 +2066,57 @@  static int libxl__vfb_and_vkb_from_hvm_guest_config(libxl__gc *gc,
     return 0;
 }
 
+static int libxl__write_stub_linux_dm_argv(libxl__gc *gc,
+                                           int dm_domid, int guest_domid,
+                                           char **args)
+{
+    const char *vm_path;
+    char *path;
+    struct xs_permissions roperm[2];
+    xs_transaction_t t = XBT_NULL;
+    int rc;
+
+    roperm[0].id = 0;
+    roperm[0].perms = XS_PERM_NONE;
+    roperm[1].id = dm_domid;
+    roperm[1].perms = XS_PERM_READ;
+
+    rc = libxl__xs_read_mandatory(gc, XBT_NULL,
+                                  GCSPRINTF("/local/domain/%d/vm", guest_domid),
+                                  &vm_path);
+    if (rc)
+        return rc;
+
+    path = GCSPRINTF("%s/image/dm-argv", vm_path);
+
+    for (;;) {
+        int i;
+
+        rc = libxl__xs_transaction_start(gc, &t);
+        if (rc) goto out;
+
+        rc = libxl__xs_mknod(gc, t, path, roperm, ARRAY_SIZE(roperm));
+        if (rc) goto out;
+
+        for (i=1; args[i] != NULL; i++) {
+            rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/%03d", path, i),
+                                         args[i]);
+            if (rc) goto out;
+        }
+
+        rc = libxl__xs_transaction_commit(gc, &t);
+        if (!rc) break;
+        if (rc<0) goto out;
+    }
+
+    return 0;
+
+ out:
+    libxl__xs_transaction_abort(gc, &t);
+
+    return rc;
+}
+
 static int libxl__write_stub_dmargs(libxl__gc *gc,
                                     int dm_domid, int guest_domid,
                                     char **args)
@@ -2275,7 +2326,10 @@  void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 
     libxl__store_libxl_entry(gc, guest_domid, "dm-version",
         libxl_device_model_version_to_string(dm_config->b_info.device_model_version));
-    libxl__write_stub_dmargs(gc, dm_domid, guest_domid, args);
+    if (libxl__stubdomain_is_linux(&guest_config->b_info))
+        libxl__write_stub_linux_dm_argv(gc, dm_domid, guest_domid, args);
+    else
+        libxl__write_stub_dmargs(gc, dm_domid, guest_domid, args);
     libxl__xs_printf(gc, XBT_NULL,
                      GCSPRINTF("%s/image/device-model-domid",
                                libxl__xs_get_dompath(gc, guest_domid)),