[PATCH for-4.6 0/8] Fix libxl TOOLSTACK records for migration v2

[PATCH for-4.6 0/8] Fix libxl TOOLSTACK records for migration v2

[Andrew Cooper]

The libxl blob for toolstack records was not 32/64bit safe.  In 64bit
builds, it had an extra 4 bytes of padding due to alignment, and because
of the pointer arithmatic used to build it, leaked 4 bytes of libxl heap
into the stream.

Respecify XENSTORE_DATA as EMULATOR_XENSTORE_DATA and take remedial
action to prevent this bitness issue escaping into a Xen release.

Patches 1-3 are bugfixes, one of which has already been posted on list.
Patches 4-8 implement EMULATOR_XENSTORE_DATA and remove the older
"toolstack" record implementation.

Andrew Cooper (8):
  tools/libxl: Only continue stream operations if the stream is still in progress
  tools/libxl: Assert that libxl__ao_inprogress_gc() is not called with NULL
  tools/libxl: Make libxl__conversion_helper_abort() safe to use
  docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA
  tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content
  tools/libxl: Prepare to write multiple records with EMULATOR headers
  libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records
  tools/libxl: Drop all legacy "toolstack" record infrastructure

 docs/specs/libxl-migration-stream.pandoc   |   24 ++-
 tools/libxl/libxl_convert_callout.c        |    2 +-
 tools/libxl/libxl_dom.c                    |  302 ++++++++++------------------
 tools/libxl/libxl_event.c                  |    1 +
 tools/libxl/libxl_internal.h               |   11 +-
 tools/libxl/libxl_sr_stream_format.h       |   10 +-
 tools/libxl/libxl_stream_read.c            |   33 ++-
 tools/libxl/libxl_stream_write.c           |  135 +++++++++----
 tools/python/scripts/convert-legacy-stream |   68 ++++++-
 tools/python/xen/migration/legacy.py       |   40 +++-
 tools/python/xen/migration/libxl.py        |   70 +++++--
 tools/python/xen/migration/tests.py        |    2 +-
 12 files changed, 407 insertions(+), 291 deletions(-)

-- 
1.7.10.4

[PATCH for-4.6 1/8] tools/libxl: Only continue stream operations if the stream is still in progress

[Andrew Cooper]

Part of the callback contract with check_all_finished() is that each
running parallel task shall call it exactly once.

Previously, it was possible for stream_continue() or
write_toolstack_record() to fail and call into check_all_finished().  As
the save helpers callback has fired, it no longer counts as in use,
which causes check_all_finished() to fire the stream callback.  Then,
unwinding the stack back and calling check_all_finished() a second time
results in the same conditions being observed, and the stream callback
being fired a second time.

To avoid this, check_all_finished() is called before any other actions
which continue the stream functionality, and the stream is only
continued if it has not been torn down.  This guarantees not to continue
stream operations if the stream does not owe a callback to
check_all_finished().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

v2.5: Extra comments explaining the situation
---
 tools/libxl/libxl_stream_read.c  |   20 ++++++++++++++------
 tools/libxl/libxl_stream_write.c |   11 +++++++++--
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
index d54789c..fd3675c 100644
--- a/tools/libxl/libxl_stream_read.c
+++ b/tools/libxl/libxl_stream_read.c
@@ -738,14 +738,22 @@ void libxl__xc_domain_restore_done(libxl__egc *egc, void *dcs_void,
         goto err;
     }
 
-    /*
-     * Libxc has indicated that it is done with the stream.  Resume reading
-     * libxl records from it.
-     */
-    stream_continue(egc, stream);
-
  err:
     check_all_finished(egc, stream, rc);
+
+    /*
+     * This function is the callback associated with the save helper
+     * task, not the stream task.  We do not know whether the stream is
+     * alive, and check_all_finished() may have torn it down around us.
+     * If the stream is not still alive, we must not continue any work.
+     */
+    if (libxl__stream_read_inuse(stream)) {
+        /*
+         * Libxc has indicated that it is done with the stream.  Resume reading
+         * libxl records from it.
+         */
+        stream_continue(egc, stream);
+    }
 }
 
 static void conversion_done(libxl__egc *egc,
diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
index 676ad0a..9e9c998 100644
--- a/tools/libxl/libxl_stream_write.c
+++ b/tools/libxl/libxl_stream_write.c
@@ -275,10 +275,17 @@ void libxl__xc_domain_save_done(libxl__egc *egc, void *dss_void,
         goto err;
     }
 
-    write_toolstack_record(egc, stream);
-
  err:
     check_all_finished(egc, stream, rc);
+
+    /*
+     * This function is the callback associated with the save helper
+     * task, not the stream task.  We do not know whether the stream is
+     * alive, and check_all_finished() may have torn it down around us.
+     * If the stream is not still alive, we must not continue any work.
+     */
+    if (libxl__stream_write_inuse(stream))
+        write_toolstack_record(egc, stream);
 }
 
 static void write_toolstack_record(libxl__egc *egc,
-- 
1.7.10.4

[PATCH for-4.6 2/8] tools/libxl: Assert that libxl__ao_inprogress_gc() is not called with NULL

[Andrew Cooper]

libxl__ao_inprogress_gc() is hidden behind various macros used to
construct local variables.  Assert() that NULL is not passed, to make
such an error very obvious, rather than a plain segfault at 0.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_event.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index 8acecfa..bfb6b31 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1862,6 +1862,7 @@ void libxl__ao_create_fail(libxl__ao *ao)
 
 libxl__gc *libxl__ao_inprogress_gc(libxl__ao *ao)
 {
+    assert(ao);
     assert(ao->magic == LIBXL__AO_MAGIC);
     assert(!ao->complete);
     return &ao->gc;
-- 
1.7.10.4

Re: [PATCH for-4.6 2/8] tools/libxl: Assert that libxl__ao_inprogress_gc() is not called with NULL

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.6 2/8] tools/libxl: Assert that libxl__ao_inprogress_gc() is not called with NULL"):
> libxl__ao_inprogress_gc() is hidden behind various macros used to
> construct local variables.  Assert() that NULL is not passed, to make
> such an error very obvious, rather than a plain segfault at 0.

I'm not in general a huge fan of this approach.  It can be unclear
where it ends: should every function that requires non-null arguments
assert all those arguments ?

But (a) libxl__ao_inprogress_gc is a special case because one of its
purposes is to check the state of the ao being dealt with and (b) the
assert may help prevent an optimiser (perhaps a whole program
optimiser) from doing something insane if it notices that a particular
path always passes ao==0.

So:

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH for-4.6 3/8] tools/libxl: Make libxl__conversion_helper_abort() safe to use

[Andrew Cooper]

STATE_AO_GC(chs->ao) uses chs->ao before determining whether the helper
is active.  In the case that the helper has not been started, its ao
will not have been set up.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

IMO, this is better than the alternative of requiring that ao is
unconditionally set up.  It allows the other functions which explicitly
shouldn't be used on a non-running helper to trip an assert() rather
than limp further along.
---
 tools/libxl/libxl_convert_callout.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_convert_callout.c b/tools/libxl/libxl_convert_callout.c
index 65b4df9..55645ba 100644
--- a/tools/libxl/libxl_convert_callout.c
+++ b/tools/libxl/libxl_convert_callout.c
@@ -118,10 +118,10 @@ void libxl__conversion_helper_abort(libxl__egc *egc,
                                     libxl__conversion_helper_state *chs,
                                     int rc)
 {
-    STATE_AO_GC(chs->ao);
     assert(rc);
 
     if (libxl__conversion_helper_inuse(chs)) {
+        STATE_AO_GC(chs->ao); /* A non-inuse chs doesn't have an ao. */
 
         if (!chs->rc)
             chs->rc = rc;
-- 
1.7.10.4

Re: [PATCH for-4.6 3/8] tools/libxl: Make libxl__conversion_helper_abort() safe to use

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.6 3/8] tools/libxl: Make libxl__conversion_helper_abort() safe to use"):
> STATE_AO_GC(chs->ao) uses chs->ao before determining whether the helper
> is active.  In the case that the helper has not been started, its ao
> will not have been set up.

I am not happy with this, because we are further complicating the
rules for the abstract state of the chs.

As I said on irc,

19:07 <Diziet> The problem here is that the rules are not written down
               clearly enough.  In this particular case the set of
               abstract states is written down.
19:07 <Diziet> The Undefined / Idle / Active thing from libxl__ev_*
               doesn't quite apply because libxl__ev_blah_register
               takes things as arguments, rather than expecting the
               caller to fill them in.

So at the very least if you are to do this I would like to see a
comment explicitly and formally setting out the abstract states of a
chs.  For examples of how to write this, look for the doc comments
which show up in searches for:
  - libxl__xs_transaction_start
  - libxl__ev_KIND_register
This comment should explain that libxl__conversion_helper_abort is
a special case.

I think you will find that trying to write this down in this form
results in your proposal requiring an extra abstract state.


Instead, I think it would be preferable for all similar operations to
all work the same.  And then, such a comment about the abstract states
could be written in a general way so that it describes
libxl__datacopier and libxl__openpty too.  In this case IMO while it
would be nice for there to be a comment, it's not essential, because
it is (very nearly) implied by the comment about libxl__ev_KIND_*,
when read together with the comment saying the caller must fill in ao.

So as I say I think it is simpler to require that the foostate->ao be
valid on all entries to libxl__foo_abort().  This is indeed the
current behaviour of:
  - libxl__datacopier_kill
  - libxl__save_helper_abort

I think it is only an accident that libxl__xswait_stop and
libxl__stream_{read,write}_abort don't dereference xswa->ao.


I would also like a comment (which is currently missing) about who is
supposed to fill in the public fields in the chs.  See for example the
comment at the top of libxl__datacopier_state.

Ian.

[PATCH for-4.6 4/8] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA

[Andrew Cooper]

The legacy "toolstack" record as implemented in libxl turns out not to
be 32/64bit safe.  As migration v2 has not shipped yet, take this
opportunity to adjust the specification and fix the incompatibility.

Libxl shall loose all knowledge of the old "toolstack" blob and use this
EMULATOR_XENSTORE_DATA record instead.  Compatibility shall be handled
by the legacy conversion script.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 docs/specs/libxl-migration-stream.pandoc   |   24 +++++++---
 tools/libxl/libxl_sr_stream_format.h       |   11 +++--
 tools/python/scripts/convert-legacy-stream |    2 +-
 tools/python/xen/migration/legacy.py       |   40 +++++++++++++++-
 tools/python/xen/migration/libxl.py        |   71 ++++++++++++++++++++--------
 tools/python/xen/migration/tests.py        |    2 +-
 6 files changed, 114 insertions(+), 36 deletions(-)

diff --git a/docs/specs/libxl-migration-stream.pandoc b/docs/specs/libxl-migration-stream.pandoc
index cdec168..db142b6 100644
--- a/docs/specs/libxl-migration-stream.pandoc
+++ b/docs/specs/libxl-migration-stream.pandoc
@@ -113,7 +113,7 @@ type         0x00000000: END
 
              0x00000001: LIBXC_CONTEXT
 
-             0x00000002: XENSTORE_DATA
+             0x00000002: EMULATOR_XENSTORE_DATA
 
              0x00000003: EMULATOR_CONTEXT
 
@@ -163,17 +163,29 @@ The libxc context record contains no fields; its body_length is 0[^1].
 might write into the stream, especially for live migration where the quantity
 of data is partially proportional to the elapsed time.
 
-XENSTORE\_DATA
--------------
+EMULATOR\_XENSTORE\_DATA
+------------------------
 
-A record containing xenstore key/value pairs of data.
+A set of xenstore key/value pairs for a specific emulator associated with the
+domain.
 
      0     1     2     3     4     5     6     7 octet
-    +-------------------------------------------------+
-    | xenstore key/value pairs                        |
+    +------------------------+------------------------+
+    | emulator_id            | index                  |
+    +------------------------+------------------------+
+    | xenstore key/value data                         |
     ...
     +-------------------------------------------------+
 
+Xenstore key and value data are encoded as a pair of NUL terminated C
+strings.  Keys shall be relative to to the device models xenstore tree for the
+new domain
+
+i.e. relative to `/local/domain/$dm_domid/device-model/$domid/`
+
+The _emulator\_id_ and _index_ have the same meaning as the
+**EMULATOR\_CONTEXT** record.
+
 EMULATOR\_CONTEXT
 ----------------
 
diff --git a/tools/libxl/libxl_sr_stream_format.h b/tools/libxl/libxl_sr_stream_format.h
index 3f3c497..4c23367 100644
--- a/tools/libxl/libxl_sr_stream_format.h
+++ b/tools/libxl/libxl_sr_stream_format.h
@@ -31,11 +31,12 @@
 /* All records must be aligned up to an 8 octet boundary */
 #define REC_ALIGN_ORDER              3U
 
-#define REC_TYPE_END                 0x00000000U
-#define REC_TYPE_LIBXC_CONTEXT       0x00000001U
-#define REC_TYPE_XENSTORE_DATA       0x00000002U
-#define REC_TYPE_EMULATOR_CONTEXT    0x00000003U
-#define REC_TYPE_CHECKPOINT_END      0x00000004U
+#define REC_TYPE_END                    0x00000000U
+#define REC_TYPE_LIBXC_CONTEXT          0x00000001U
+#define REC_TYPE_XENSTORE_DATA          0x00000002U /* TOOLSTACK COMPAT */
+#define REC_TYPE_EMULATOR_XENSTORE_DATA 0x00000002U
+#define REC_TYPE_EMULATOR_CONTEXT       0x00000003U
+#define REC_TYPE_CHECKPOINT_END         0x00000004U
 
 typedef struct libxl__sr_emulator_hdr
 {
diff --git a/tools/python/scripts/convert-legacy-stream b/tools/python/scripts/convert-legacy-stream
index d54fa22..16331a4 100755
--- a/tools/python/scripts/convert-legacy-stream
+++ b/tools/python/scripts/convert-legacy-stream
@@ -174,7 +174,7 @@ def write_libxl_xenstore_data(data):
 
 def write_libxl_emulator_context(blob):
     write_record(libxl.REC_TYPE_emulator_context,
-                 pack(libxl.EMULATOR_CONTEXT_FORMAT,
+                 pack(libxl.EMULATOR_HEADER_FORMAT,
                       libxl.EMULATOR_ID_unknown, 0) + blob)
 
 def rdexact(nr_bytes):
diff --git a/tools/python/xen/migration/legacy.py b/tools/python/xen/migration/legacy.py
index 2f2240a..6456d61 100644
--- a/tools/python/xen/migration/legacy.py
+++ b/tools/python/xen/migration/legacy.py
@@ -2,12 +2,15 @@
 # -*- coding: utf-8 -*-
 
 """
-Libxc legacy migration streams
+Legacy migration stream information.
 
-Documentation and record structures for legacy migration
+Documentation and record structures for legacy migration, for both libxc
+and libxl.
 """
 
 """
+Libxc:
+
 SAVE/RESTORE/MIGRATE PROTOCOL
 =============================
 
@@ -277,3 +280,36 @@ MAX_BATCH = 1024
 
 # Maximum #VCPUs currently supported for save/restore
 MAX_VCPU_ID = 4095
+
+
+"""
+Libxl:
+
+Legacy "toolstack" record layout:
+
+Version 1:
+  uint32_t version
+  QEMU physmap data:
+    uint32_t count
+    libxl__physmap_info * count
+
+The problem is that libxl__physmap_info was declared as:
+
+struct libxl__physmap_info {
+    uint64_t phys_offset;
+    uint64_t start_addr;
+    uint64_t size;
+    uint32_t namelen;
+    char name[];
+};
+
+Which has 4 bytes of padding at the end in a 64bit build, thus not the
+same between 32 and 64bit builds.
+
+Because of the pointer arithmatic used to construct the record, the 'name' was
+shifted up to start at the padding, leaving the erronious 4 bytes at the end
+of the name string, after the NUL terminator.
+
+Instead, the information described here has been changed to fit in a new
+EMULATOR_XENSTORE_DATA record made of NUL terminated strings.
+"""
diff --git a/tools/python/xen/migration/libxl.py b/tools/python/xen/migration/libxl.py
index 415502e..1a9ca87 100644
--- a/tools/python/xen/migration/libxl.py
+++ b/tools/python/xen/migration/libxl.py
@@ -10,7 +10,7 @@ verification routines.
 
 import sys
 
-from struct import calcsize, unpack
+from struct import calcsize, unpack, unpack_from
 from xen.migration.verify import StreamError, RecordError, VerifyBase
 from xen.migration.libxc import VerifyLibxc
 
@@ -32,22 +32,23 @@ HDR_OPT_RESZ_MASK = 0xfffc
 # Records
 RH_FORMAT = "II"
 
-REC_TYPE_end              = 0x00000000
-REC_TYPE_libxc_context    = 0x00000001
-REC_TYPE_xenstore_data    = 0x00000002
-REC_TYPE_emulator_context = 0x00000003
-REC_TYPE_checkpoint_end   = 0x00000004
+REC_TYPE_end                    = 0x00000000
+REC_TYPE_libxc_context          = 0x00000001
+REC_TYPE_xenstore_data          = 0x00000002 # TOOLSTACK COMPAT
+REC_TYPE_emulator_xenstore_data = 0x00000002
+REC_TYPE_emulator_context       = 0x00000003
+REC_TYPE_checkpoint_end         = 0x00000004
 
 rec_type_to_str = {
-    REC_TYPE_end              : "End",
-    REC_TYPE_libxc_context    : "Libxc context",
-    REC_TYPE_xenstore_data    : "Xenstore data",
-    REC_TYPE_emulator_context : "Emulator context",
-    REC_TYPE_checkpoint_end   : "Checkpoint end",
+    REC_TYPE_end                    : "End",
+    REC_TYPE_libxc_context          : "Libxc context",
+    REC_TYPE_emulator_xenstore_data : "Emulator xenstore data",
+    REC_TYPE_emulator_context       : "Emulator context",
+    REC_TYPE_checkpoint_end         : "Checkpoint end",
 }
 
-# emulator_context
-EMULATOR_CONTEXT_FORMAT = "II"
+# emulator_* header
+EMULATOR_HEADER_FORMAT = "II"
 
 EMULATOR_ID_unknown       = 0x00000000
 EMULATOR_ID_qemu_trad     = 0x00000001
@@ -155,22 +156,50 @@ class VerifyLibxl(VerifyBase):
         VerifyLibxc(self.info, self.read).verify()
 
 
-    def verify_record_xenstore_data(self, content):
-        """ Xenstore Data record """
+    def verify_record_emulator_xenstore_data(self, content):
+        """ Emulator Xenstore Data record """
+        minsz = calcsize(EMULATOR_HEADER_FORMAT)
 
-        if len(content) == 0:
-            raise RecordError("Xenstore data record with zero length")
+        if len(content) < minsz:
+            raise RecordError("Length must be at least %d bytes, got %d"
+                              % (minsz, len(content)))
+
+        emu_id, emu_idx = unpack(EMULATOR_HEADER_FORMAT, content[:minsz])
+
+        if emu_id not in emulator_id_to_str:
+            raise RecordError("Unrecognised emulator id 0x%x" % (emu_id, ))
+
+        self.info("Emulator Xenstore Data (%s, idx %d)"
+                  % (emulator_id_to_str[emu_id], emu_idx))
+
+        # Chop off the emulator header
+        content = content[minsz:]
+
+        if len(content):
+
+            if content[-1] != '\x00':
+                raise RecordError("Data not NUL terminated")
+
+            # Split without the final NUL, to get an even number of parts
+            parts = content[:-1].split("\x00")
+
+            if (len(parts) % 2) != 0:
+                raise RecordError("Expected an even number of strings, got %d"
+                                  % (len(parts), ))
+
+            for key, val in zip(parts[0::2], parts[1::2]):
+                self.info("  '%s' = '%s'" % (key, val))
 
 
     def verify_record_emulator_context(self, content):
         """ Emulator Context record """
-        minsz = calcsize(EMULATOR_CONTEXT_FORMAT)
+        minsz = calcsize(EMULATOR_HEADER_FORMAT)
 
         if len(content) < minsz:
             raise RecordError("Length must be at least %d bytes, got %d"
                               % (minsz, len(content)))
 
-        emu_id, emu_idx = unpack(EMULATOR_CONTEXT_FORMAT, content[:minsz])
+        emu_id, emu_idx = unpack(EMULATOR_HEADER_FORMAT, content[:minsz])
 
         if emu_id not in emulator_id_to_str:
             raise RecordError("Unrecognised emulator id 0x%x" % (emu_id, ))
@@ -190,8 +219,8 @@ record_verifiers = {
         VerifyLibxl.verify_record_end,
     REC_TYPE_libxc_context:
         VerifyLibxl.verify_record_libxc_context,
-    REC_TYPE_xenstore_data:
-        VerifyLibxl.verify_record_xenstore_data,
+    REC_TYPE_emulator_xenstore_data:
+        VerifyLibxl.verify_record_emulator_xenstore_data,
     REC_TYPE_emulator_context:
         VerifyLibxl.verify_record_emulator_context,
     REC_TYPE_checkpoint_end:
diff --git a/tools/python/xen/migration/tests.py b/tools/python/xen/migration/tests.py
index 91044cd..026cf38 100644
--- a/tools/python/xen/migration/tests.py
+++ b/tools/python/xen/migration/tests.py
@@ -37,7 +37,7 @@ class TestLibxl(unittest.TestCase):
         for fmt, sz in ( (libxl.HDR_FORMAT, 16),
                          (libxl.RH_FORMAT, 8),
 
-                         (libxl.EMULATOR_CONTEXT_FORMAT, 8),
+                         (libxl.EMULATOR_HEADER_FORMAT, 8),
                          ):
             self.assertEqual(calcsize(fmt), sz)
 
-- 
1.7.10.4

Re: [PATCH for-4.6 4/8] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA

[David Vrabel]

On 28/07/15 22:44, Andrew Cooper wrote:
> The legacy "toolstack" record as implemented in libxl turns out not to
> be 32/64bit safe.  As migration v2 has not shipped yet, take this
> opportunity to adjust the specification and fix the incompatibility.
> 
> Libxl shall loose all knowledge of the old "toolstack" blob and use this
> EMULATOR_XENSTORE_DATA record instead.  Compatibility shall be handled
> by the legacy conversion script.
[...]
> +EMULATOR\_XENSTORE\_DATA
> +------------------------
>  
> -A record containing xenstore key/value pairs of data.
> +A set of xenstore key/value pairs for a specific emulator associated with the
> +domain.
>  
>       0     1     2     3     4     5     6     7 octet
> -    +-------------------------------------------------+
> -    | xenstore key/value pairs                        |
> +    +------------------------+------------------------+
> +    | emulator_id            | index                  |
> +    +------------------------+------------------------+
> +    | xenstore key/value data                         |
>      ...
>      +-------------------------------------------------+
>  
> +Xenstore key and value data are encoded as a pair of NUL terminated C
> +strings.  Keys shall be relative to to the device models xenstore tree for the
> +new domain

This isn't quite descriptive enough, suggest:

   "Xenstore key/value data is encoded as a packed sequence of (key,
    value) tuples.  Each (key, value) tuple is a packed pair of NUL
    terminated UTF-8 encoded character strings.  The keys are relative
    to..."

In particular, it is essential that character strings have a well
defined encoding (I always recommend UTF-8) but the Xenstore protocol
may specify a different encoding (perhaps ASCII?).  The data may also be
better specified as a NULL-terminated octet sequence (rather than
characters).

> +
> +i.e. relative to `/local/domain/$dm_domid/device-model/$domid/`
> +
> +The _emulator\_id_ and _index_ have the same meaning as the
> +**EMULATOR\_CONTEXT** record.
> +
>  EMULATOR\_CONTEXT
>  ----------------
>  

Re: [PATCH for-4.6 4/8] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA

[Andrew Cooper]

On 29/07/15 10:35, David Vrabel wrote:
>
>> +EMULATOR\_XENSTORE\_DATA
>> +------------------------
>>  
>> -A record containing xenstore key/value pairs of data.
>> +A set of xenstore key/value pairs for a specific emulator associated with the
>> +domain.
>>  
>>       0     1     2     3     4     5     6     7 octet
>> -    +-------------------------------------------------+
>> -    | xenstore key/value pairs                        |
>> +    +------------------------+------------------------+
>> +    | emulator_id            | index                  |
>> +    +------------------------+------------------------+
>> +    | xenstore key/value data                         |
>>      ...
>>      +-------------------------------------------------+
>>  
>> +Xenstore key and value data are encoded as a pair of NUL terminated C
>> +strings.  Keys shall be relative to to the device models xenstore tree for the
>> +new domain
> This isn't quite descriptive enough, suggest:
>
>    "Xenstore key/value data is encoded as a packed sequence of (key,
>     value) tuples.  Each (key, value) tuple is a packed pair of NUL
>     terminated UTF-8 encoded character strings.  The keys are relative
>     to..."
>
> In particular, it is essential that character strings have a well
> defined encoding (I always recommend UTF-8) but the Xenstore protocol
> may specify a different encoding (perhaps ASCII?).

The best I can find is from xenstore.txt which says:

While xenstore and most tools and APIs are capable of dealing with
arbitrary binary data as values, this should generally be avoided.
Data should generally be human-readable for ease of management and
debugging; xenstore is not a high-performance facility and should be
used only for small amounts of control plane data.  Therefore xenstore
values should normally be 7-bit ASCII text strings containing bytes
0x20..0x7f only, and should not contain a trailing nul byte.

I don't think it is wise to constrain the record format further than
xenstore permits.

>   The data may also be
> better specified as a NULL-terminated octet sequence (rather than
> characters).

I will opt for octet sequence, and state that it should be encoded
suitably for xenstore, without actually being more specific.

~Andrew

Re: [PATCH for-4.6 4/8] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.6 4/8] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA"):
> The legacy "toolstack" record as implemented in libxl turns out not to
> be 32/64bit safe.  As migration v2 has not shipped yet, take this
> opportunity to adjust the specification and fix the incompatibility.
> 
> Libxl shall loose all knowledge of the old "toolstack" blob and use this
> EMULATOR_XENSTORE_DATA record instead.  Compatibility shall be handled
> by the legacy conversion script.
...
> -A record containing xenstore key/value pairs of data.
> +A set of xenstore key/value pairs for a specific emulator associated with \
the
> +domain.

Wrap damage (and elsewhere in this doc).

But, aside from that, this looks good.

Ian.

Re: [PATCH for-4.6 4/8] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA

[Andrew Cooper]

On 29/07/15 12:28, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH for-4.6 4/8] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA"):
>> The legacy "toolstack" record as implemented in libxl turns out not to
>> be 32/64bit safe.  As migration v2 has not shipped yet, take this
>> opportunity to adjust the specification and fix the incompatibility.
>>
>> Libxl shall loose all knowledge of the old "toolstack" blob and use this
>> EMULATOR_XENSTORE_DATA record instead.  Compatibility shall be handled
>> by the legacy conversion script.
> ...
>> -A record containing xenstore key/value pairs of data.
>> +A set of xenstore key/value pairs for a specific emulator associated with \
> the
>> +domain.
> Wrap damage (and elsewhere in this doc).

The entirely of this document is wrapped at 78 character, like all other
work I do in the Xen tree.

Furthermore this is compatible with both the Xen CODING_STYLE (< 80) and
the libxl CODING_STYLE (75-80).

~Andrew

[PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content

[Andrew Cooper]

The new EMULATOR_XENSTORE_DATA content is a sequence of NUL terminated
key/value strings, with the key relative to the device model's xenstore
tree.

A sample might look like (as decoded by verify-stream-v2):

    Emulator Xenstore Data (Qemu Upstream, idx 0)
      'physmap/1f00000/start_addr' = 'f0000000'
      'physmap/1f00000/size' = '800000'
      'physmap/1f00000/name' = 'vga.vram'

This patch introduces libxl helpers to save and restore this new format.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dom.c      |  141 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |    4 ++
 2 files changed, 145 insertions(+)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 5555fea..885eb5e 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1151,6 +1151,65 @@ int libxl__toolstack_restore(uint32_t domid, const uint8_t *ptr,
     return ret;
 }
 
+int libxl__restore_emulator_xenstore_data(libxl__domain_create_state *dcs,
+                                          const char *ptr, uint32_t size)
+{
+    STATE_AO_GC(dcs->ao);
+    const char *key = ptr, *val;
+    int rc;
+
+    const uint32_t domid = dcs->guest_domid;
+    const uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
+    const char *xs_root = libxl__device_model_xs_path(gc, dm_domid, domid, "");
+
+    while ( size ) {
+        /* Sanitise key. */
+        size_t keylen = strnlen(key, size);
+
+        if ( keylen == size ) {
+            rc = ERROR_FAIL;
+            LOG(ERROR, "Key in xenstore data not NUL terminated");
+            goto out;
+        }
+        if ( keylen == 0 ) {
+            rc = ERROR_FAIL;
+            LOG(ERROR, "NULL key found in xenstore data");
+            goto out;
+        }
+        if ( key[0] == '/' ) {
+            rc = ERROR_FAIL;
+            LOG(ERROR, "Key in xenstore data not relative");
+            goto out;
+        }
+
+        size -= (keylen + 1);
+
+        /* The val string starts after the NUL terminator of key. */
+        val = key + keylen + 1;
+
+        /* Sanitise val. */
+        size_t vallen = strnlen(val, size);
+
+        if ( vallen == size ) {
+            rc = ERROR_FAIL;
+            LOG(ERROR, "Val in xenstore data not NUL terminated");
+            goto out;
+        }
+
+        size -= (vallen + 1);
+
+        libxl__xs_write(gc, 0, GCSPRINTF("%s/%s", xs_root, key), "%s", val);
+
+        /* Move key forwards over the key and value pair just written. */
+        key = key + keylen + vallen + 2;
+    }
+
+    rc = 0;
+
+ out:
+    return rc;
+}
+
 /*==================== Domain suspend (save) ====================*/
 
 static void stream_done(libxl__egc *egc,
@@ -1487,6 +1546,88 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
     return ret;
 }
 
+int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
+                                       uint8_t **callee_buf,
+                                       uint32_t *callee_len)
+{
+    STATE_AO_GC(dss->ao);
+    const char *xs_path;
+    char **entries, *ptr, *buf = NULL;
+    unsigned int nr_entries, rel_start, i, len = 0;
+    int rc;
+
+    const uint32_t domid = dss->domid;
+    const uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
+
+    xs_path = libxl__device_model_xs_path(gc, dm_domid, domid, "/physmap");
+    if (!xs_path) { rc = 0; goto out; }
+
+    /* &foo[rel_start] is the xenstore path starting at the 'p' of physmap */
+    rel_start = strlen(xs_path) - 7;
+
+    entries = libxl__xs_directory(gc, 0, xs_path, &nr_entries);
+    if (!entries || nr_entries == 0) { rc = 0; goto out; }
+
+    for (i = 0; i < nr_entries; ++i) {
+        const char *start_addr, *start_addr_val,
+            *size, *size_val, *name, *name_val;
+
+        start_addr = libxl__device_model_xs_path(
+            gc, dm_domid, domid, "/physmap/%s/start_addr", entries[i]);
+        size = libxl__device_model_xs_path(
+            gc, dm_domid, domid, "/physmap/%s/size", entries[i]);
+        name = libxl__device_model_xs_path(
+            gc, dm_domid, domid, "/physmap/%s/name", entries[i]);
+
+        if (!start_addr || !size || !name) { rc = ERROR_FAIL; goto out; }
+
+        start_addr_val = libxl__xs_read(gc, 0, start_addr);
+        size_val = libxl__xs_read(gc, 0, size);
+        name_val = libxl__xs_read(gc, 0, name);
+
+        if (!start_addr_val || !size_val || !name_val) {
+            rc = ERROR_FAIL; goto out;
+        }
+
+        /*
+         * Appends 's' to 'buf', including a NUL teriminator.  Mutates
+         * 'buf', 'ptr' and 'len' in scope.
+         */
+#define APPEND_STRING(s) ({                                     \
+                size_t sz = strlen(s) + 1;                      \
+                ptr = realloc(buf, len + sz);                   \
+                if (!ptr) { rc = ERROR_NOMEM; goto out; }       \
+                buf = ptr;                                      \
+                strcpy(&buf[len], s);                           \
+                len += sz;                                      \
+                buf[len - 1] = '\0';                            \
+            })
+
+        APPEND_STRING(&start_addr[rel_start]);
+        APPEND_STRING(start_addr_val);
+
+        APPEND_STRING(&size[rel_start]);
+        APPEND_STRING(size_val);
+
+        APPEND_STRING(&name[rel_start]);
+        APPEND_STRING(name_val);
+
+#undef APPEND_STRING
+    }
+
+    rc = 0;
+
+ out:
+    if (rc)
+        free(buf);
+    else {
+        *callee_buf = (uint8_t *)buf;
+        *callee_len = len;
+    }
+
+    return rc;
+}
+
 /*----- remus callbacks -----*/
 static void remus_domain_suspend_callback_common_done(libxl__egc *egc,
                                 libxl__domain_suspend_state *dss, int ok);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 911de2d..8426221 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3439,6 +3439,10 @@ void libxl__xc_domain_saverestore_async_callback_done(libxl__egc *egc,
                                (int domid, unsigned int enable, void *data);
 _hidden int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
         uint32_t *len, void *data);
+_hidden int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
+                                               uint8_t **buf, uint32_t *len);
+_hidden int libxl__restore_emulator_xenstore_data(
+    libxl__domain_create_state *dcs, const char *ptr, uint32_t size);
 
 
 /* calls libxl__xc_domain_restore_done when done */
-- 
1.7.10.4

Re: [PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content

[Wei Liu]

On Tue, Jul 28, 2015 at 10:44:40PM +0100, Andrew Cooper wrote:
> The new EMULATOR_XENSTORE_DATA content is a sequence of NUL terminated
> key/value strings, with the key relative to the device model's xenstore
> tree.
> 
> A sample might look like (as decoded by verify-stream-v2):
> 
>     Emulator Xenstore Data (Qemu Upstream, idx 0)
>       'physmap/1f00000/start_addr' = 'f0000000'
>       'physmap/1f00000/size' = '800000'
>       'physmap/1f00000/name' = 'vga.vram'
> 
> This patch introduces libxl helpers to save and restore this new format.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_dom.c      |  141 ++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_internal.h |    4 ++
>  2 files changed, 145 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 5555fea..885eb5e 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1151,6 +1151,65 @@ int libxl__toolstack_restore(uint32_t domid, const uint8_t *ptr,
>      return ret;
>  }
>  
> +int libxl__restore_emulator_xenstore_data(libxl__domain_create_state *dcs,
> +                                          const char *ptr, uint32_t size)
> +{
> +    STATE_AO_GC(dcs->ao);
> +    const char *key = ptr, *val;
> +    int rc;
> +
> +    const uint32_t domid = dcs->guest_domid;
> +    const uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
> +    const char *xs_root = libxl__device_model_xs_path(gc, dm_domid, domid, "");
> +
> +    while ( size ) {

Coding style issues in this function.

> +        /* Sanitise key. */
> +        size_t keylen = strnlen(key, size);
> +
> +        if ( keylen == size ) {
> +            rc = ERROR_FAIL;
> +            LOG(ERROR, "Key in xenstore data not NUL terminated");
> +            goto out;
> +        }
> +        if ( keylen == 0 ) {
> +            rc = ERROR_FAIL;
> +            LOG(ERROR, "NULL key found in xenstore data");
> +            goto out;
> +        }
> +        if ( key[0] == '/' ) {
> +            rc = ERROR_FAIL;
> +            LOG(ERROR, "Key in xenstore data not relative");
> +            goto out;
> +        }
> +
> +        size -= (keylen + 1);
> +
> +        /* The val string starts after the NUL terminator of key. */
> +        val = key + keylen + 1;
> +
> +        /* Sanitise val. */
> +        size_t vallen = strnlen(val, size);
> +
> +        if ( vallen == size ) {
> +            rc = ERROR_FAIL;
> +            LOG(ERROR, "Val in xenstore data not NUL terminated");
> +            goto out;
> +        }
> +
> +        size -= (vallen + 1);
> +
> +        libxl__xs_write(gc, 0, GCSPRINTF("%s/%s", xs_root, key), "%s", val);
> +
> +        /* Move key forwards over the key and value pair just written. */
> +        key = key + keylen + vallen + 2;
> +    }
> +
> +    rc = 0;
> +
> + out:
> +    return rc;
> +}
> +
>  /*==================== Domain suspend (save) ====================*/
>  
>  static void stream_done(libxl__egc *egc,
> @@ -1487,6 +1546,88 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
>      return ret;
>  }
>  
> +int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
> +                                       uint8_t **callee_buf,
> +                                       uint32_t *callee_len)
> +{
> +    STATE_AO_GC(dss->ao);
> +    const char *xs_path;
> +    char **entries, *ptr, *buf = NULL;
> +    unsigned int nr_entries, rel_start, i, len = 0;
> +    int rc;
> +
> +    const uint32_t domid = dss->domid;
> +    const uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
> +
> +    xs_path = libxl__device_model_xs_path(gc, dm_domid, domid, "/physmap");
> +    if (!xs_path) { rc = 0; goto out; }
> +
> +    /* &foo[rel_start] is the xenstore path starting at the 'p' of physmap */
> +    rel_start = strlen(xs_path) - 7;
> +
> +    entries = libxl__xs_directory(gc, 0, xs_path, &nr_entries);
> +    if (!entries || nr_entries == 0) { rc = 0; goto out; }
> +
> +    for (i = 0; i < nr_entries; ++i) {
> +        const char *start_addr, *start_addr_val,
> +            *size, *size_val, *name, *name_val;
> +
> +        start_addr = libxl__device_model_xs_path(
> +            gc, dm_domid, domid, "/physmap/%s/start_addr", entries[i]);
> +        size = libxl__device_model_xs_path(
> +            gc, dm_domid, domid, "/physmap/%s/size", entries[i]);
> +        name = libxl__device_model_xs_path(
> +            gc, dm_domid, domid, "/physmap/%s/name", entries[i]);
> +
> +        if (!start_addr || !size || !name) { rc = ERROR_FAIL; goto out; }
> +
> +        start_addr_val = libxl__xs_read(gc, 0, start_addr);
> +        size_val = libxl__xs_read(gc, 0, size);
> +        name_val = libxl__xs_read(gc, 0, name);

XBT_NULL in libxl__xs_read please.

With those issues fixed:

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content"):
> The new EMULATOR_XENSTORE_DATA content is a sequence of NUL terminated
> key/value strings, with the key relative to the device model's xenstore
> tree.
> 
> A sample might look like (as decoded by verify-stream-v2):
> 
>     Emulator Xenstore Data (Qemu Upstream, idx 0)
>       'physmap/1f00000/start_addr' = 'f0000000'
>       'physmap/1f00000/size' = '800000'
>       'physmap/1f00000/name' = 'vga.vram'
> 
> This patch introduces libxl helpers to save and restore this new format.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_dom.c      |  141 ++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_internal.h |    4 ++
>  2 files changed, 145 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 5555fea..885eb5e 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -1151,6 +1151,65 @@ int libxl__toolstack_restore(uint32_t domid, const uint8_t *ptr,
>      return ret;
>  }
>  
> +int libxl__restore_emulator_xenstore_data(libxl__domain_create_state *dcs,
> +                                          const char *ptr, uint32_t size)
> +{
> +    STATE_AO_GC(dcs->ao);
> +    const char *key = ptr, *val;
> +    int rc;
> +
> +    const uint32_t domid = dcs->guest_domid;
> +    const uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
> +    const char *xs_root = libxl__device_model_xs_path(gc, dm_domid, domid\
, "");

I see wrap damage due to your overly long lines.  (Throughout.)

> +
> +    while ( size ) {

Coding style.  (Throughout.)

> +        /* Sanitise key. */
> +        size_t keylen = strnlen(key, size);

I think this code suggests to me that the format chosen is not very
nice.  But, anyway:

Can we please have a black box subfunction which extracts the next
nul-terminated string from this buffer ?  That would isolate all the
pointer arithmetic and give it a formal interface.

You might consider whether it is better to work with
  const char *end = ptr + size;
since that avoids the need to remember to update both ptr and size
whenever walking through the array (a common source of security bugs).

> +int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
> +                                       uint8_t **callee_buf,
> +                                       uint32_t *callee_len)
> +{
...
> +    /* &foo[rel_start] is the xenstore path starting at the 'p' of physmap */
> +    rel_start = strlen(xs_path) - 7;

This is horrible.

> +    entries = libxl__xs_directory(gc, 0, xs_path, &nr_entries);
> +    if (!entries || nr_entries == 0) { rc = 0; goto out; }
> +
> +    for (i = 0; i < nr_entries; ++i) {
> +        const char *start_addr, *start_addr_val,
> +            *size, *size_val, *name, *name_val;
> +
> +        start_addr = libxl__device_model_xs_path(
> +            gc, dm_domid, domid, "/physmap/%s/start_addr", entries[i]);
> +        size = libxl__device_model_xs_path(
> +            gc, dm_domid, domid, "/physmap/%s/size", entries[i]);
> +        name = libxl__device_model_xs_path(
> +            gc, dm_domid, domid, "/physmap/%s/name", entries[i]);

I don't understand why you don't copy the whole subdirectory.  This
needs to be explained.

If you do want to copy only some of the keys, this thing where you do
tiny bits of the work, each time three times, is a very strange
structure.

> +        /*
> +         * Appends 's' to 'buf', including a NUL teriminator.  Mutates
> +         * 'buf', 'ptr' and 'len' in scope.
> +         */
> +#define APPEND_STRING(s) ({                                     \

Please, there is no need for this to be a macro.  If you need it, make
it a function.  Yes, you'll have to pass some of its state to it.  But
that means you may (usefully) write down what its assumptions are.

Also, if you restructure this I think this macro will vanish.

> +                size_t sz = strlen(s) + 1;                      \
> +                ptr = realloc(buf, len + sz);                   \
> +                if (!ptr) { rc = ERROR_NOMEM; goto out; }       \

Please use the gc.  I guess you are going to tell me that you can't
use the gc because of the remus memory leak problem.  Do I need to go
and retrofit the per-iteration sub-ao myself ?

> +_hidden int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
> +                                               uint8_t **buf, uint32_t *len);
> +_hidden int libxl__restore_emulator_xenstore_data(
> +    libxl__domain_create_state *dcs, const char *ptr, uint32_t size);

( in wrong place.

Ian.

Re: [PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content

[Andrew Cooper]

On 29/07/15 12:49, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content"):
>> The new EMULATOR_XENSTORE_DATA content is a sequence of NUL terminated
>> key/value strings, with the key relative to the device model's xenstore
>> tree.
>>
>> A sample might look like (as decoded by verify-stream-v2):
>>
>>     Emulator Xenstore Data (Qemu Upstream, idx 0)
>>       'physmap/1f00000/start_addr' = 'f0000000'
>>       'physmap/1f00000/size' = '800000'
>>       'physmap/1f00000/name' = 'vga.vram'
>>
>> This patch introduces libxl helpers to save and restore this new format.
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/libxl/libxl_dom.c      |  141 ++++++++++++++++++++++++++++++++++++++++++
>>  tools/libxl/libxl_internal.h |    4 ++
>>  2 files changed, 145 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> index 5555fea..885eb5e 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
>> @@ -1151,6 +1151,65 @@ int libxl__toolstack_restore(uint32_t domid, const uint8_t *ptr,
>>      return ret;
>>  }
>>  
>> +int libxl__restore_emulator_xenstore_data(libxl__domain_create_state *dcs,
>> +                                          const char *ptr, uint32_t size)
>> +{
>> +    STATE_AO_GC(dcs->ao);
>> +    const char *key = ptr, *val;
>> +    int rc;
>> +
>> +    const uint32_t domid = dcs->guest_domid;
>> +    const uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
>> +    const char *xs_root = libxl__device_model_xs_path(gc, dm_domid, domid\
> , "");
>
> I see wrap damage due to your overly long lines.  (Throughout.)
>
>> +
>> +    while ( size ) {
> Coding style.  (Throughout.)
>
>> +        /* Sanitise key. */
>> +        size_t keylen = strnlen(key, size);
> I think this code suggests to me that the format chosen is not very
> nice.  But, anyway:
>
> Can we please have a black box subfunction which extracts the next
> nul-terminated string from this buffer ?  That would isolate all the
> pointer arithmetic and give it a formal interface.
>
> You might consider whether it is better to work with
>   const char *end = ptr + size;
> since that avoids the need to remember to update both ptr and size
> whenever walking through the array (a common source of security bugs).
>
>> +int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
>> +                                       uint8_t **callee_buf,
>> +                                       uint32_t *callee_len)
>> +{
> ...
>> +    /* &foo[rel_start] is the xenstore path starting at the 'p' of physmap */
>> +    rel_start = strlen(xs_path) - 7;
> This is horrible.

What do you recommend instead?

>
>> +    entries = libxl__xs_directory(gc, 0, xs_path, &nr_entries);
>> +    if (!entries || nr_entries == 0) { rc = 0; goto out; }
>> +
>> +    for (i = 0; i < nr_entries; ++i) {
>> +        const char *start_addr, *start_addr_val,
>> +            *size, *size_val, *name, *name_val;
>> +
>> +        start_addr = libxl__device_model_xs_path(
>> +            gc, dm_domid, domid, "/physmap/%s/start_addr", entries[i]);
>> +        size = libxl__device_model_xs_path(
>> +            gc, dm_domid, domid, "/physmap/%s/size", entries[i]);
>> +        name = libxl__device_model_xs_path(
>> +            gc, dm_domid, domid, "/physmap/%s/name", entries[i]);
> I don't understand why you don't copy the whole subdirectory.  This
> needs to be explained.
>
> If you do want to copy only some of the keys, this thing where you do
> tiny bits of the work, each time three times, is a very strange
> structure.

I am replicating the behaviour of the old libxl__toolstack_save(), not
redesigning the logic from scratch.  (In an ideal world, all this cruft
would be limited to qemu alone, which is the sole producer and consumer,
rather than split across qemu, xenstore and libxl, but that boat has
long since sailed.)

>
>> +        /*
>> +         * Appends 's' to 'buf', including a NUL teriminator.  Mutates
>> +         * 'buf', 'ptr' and 'len' in scope.
>> +         */
>> +#define APPEND_STRING(s) ({                                     \
> Please, there is no need for this to be a macro.  If you need it, make
> it a function.  Yes, you'll have to pass some of its state to it.  But
> that means you may (usefully) write down what its assumptions are.
>
> Also, if you restructure this I think this macro will vanish.
>
>> +                size_t sz = strlen(s) + 1;                      \
>> +                ptr = realloc(buf, len + sz);                   \
>> +                if (!ptr) { rc = ERROR_NOMEM; goto out; }       \
> Please use the gc.  I guess you are going to tell me that you can't
> use the gc because of the remus memory leak problem.  Do I need to go
> and retrofit the per-iteration sub-ao myself ?

Again, because that what older toolstack_save() did, but without the
buggy use of "ptr = realloc(ptr, ...".

On a separate thread in the 4.7 timeframe, we need to discuss the
current libxl__$FOOalloc() infrastructure.  What it currently does is
inappropriate in any daemonised context, or in any language with its own
garbage collector.

~Andrew

Re: [PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content"):
> On 29/07/15 12:49, Ian Jackson wrote:
> >> +    rel_start = strlen(xs_path) - 7;
> > This is horrible.
> 
> What do you recommend instead?

I don't see why it is necessary to do something like rel_start at all.

This whole patch could probably be made much simpler with something
like

   static const char *const physmap_entries[] = {
       "start_addr", "size", "name", NULL
   };

and then loop over it a few times.

> > I don't understand why you don't copy the whole subdirectory.  This
> > needs to be explained.
> 
> I am replicating the behaviour of the old libxl__toolstack_save(), not
> redesigning the logic from scratch.  (In an ideal world, all this cruft
> would be limited to qemu alone, which is the sole producer and consumer,
> rather than split across qemu, xenstore and libxl, but that boat has
> long since sailed.)

I guess during the freeze, that is a good explanation.

> > If you do want to copy only some of the keys, this thing where you do
> > tiny bits of the work, each time three times, is a very strange
> > structure.

But this comment stands.

> >> +                size_t sz = strlen(s) + 1;                      \
> >> +                ptr = realloc(buf, len + sz);                   \
> >> +                if (!ptr) { rc = ERROR_NOMEM; goto out; }       \
> >
> > Please use the gc.  I guess you are going to tell me that you can't
> > use the gc because of the remus memory leak problem.  Do I need to go
> > and retrofit the per-iteration sub-ao myself ?
> 
> Again, because that what older toolstack_save() did, but without the
> buggy use of "ptr = realloc(ptr, ...".

Well, I'm too late now to insist on the sub-ao, because I'm going away
for a week and a bit starting Monday.

So I guess I will have to live with the non-use of the gc.

Ian.

Re: [PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content

[Andrew Cooper]

On 31/07/15 17:34, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content"):
>> On 29/07/15 12:49, Ian Jackson wrote:
>>>> +    rel_start = strlen(xs_path) - 7;
>>> This is horrible.
>> What do you recommend instead?
> I don't see why it is necessary to do something like rel_start at all.
>
> This whole patch could probably be made much simpler with something
> like
>
>    static const char *const physmap_entries[] = {
>        "start_addr", "size", "name", NULL
>    };
>
> and then loop over it a few times.

But the rel_path has nothing to do with which subkeys get chosen.

It is to strip out the current domains information from
/local/domain/$dm_domid/device-model/$domid/.

This is because both of the domid in the path will be different on the
other side of migration.  This is why EMULATOR_XENSTORE_DATA is
purposefully specified relative to the above path, rather than as
absolute paths.

~Andrew

Re: [PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content

[Ian Campbell]

On Fri, 2015-07-31 at 17:38 +0100, Andrew Cooper wrote:
> On 31/07/15 17:34, Ian Jackson wrote:
> > Andrew Cooper writes ("Re: [PATCH for-4.6 5/8] tools/libxl: Save and 
> > restore EMULATOR_XENSTORE_DATA content"):
> > > On 29/07/15 12:49, Ian Jackson wrote:
> > > > > +    rel_start = strlen(xs_path) - 7;
> > > > This is horrible.
> > > What do you recommend instead?
> > I don't see why it is necessary to do something like rel_start at all.
> > 
> > This whole patch could probably be made much simpler with something
> > like
> > 
> >    static const char *const physmap_entries[] = {
> >        "start_addr", "size", "name", NULL
> >    };
> > 
> > and then loop over it a few times.
> 
> But the rel_path has nothing to do with which subkeys get chosen.
> 
> It is to strip out the current domains information from
> /local/domain/$dm_domid/device-model/$domid/.
> 
> This is because both of the domid in the path will be different on the
> other side of migration.  This is why EMULATOR_XENSTORE_DATA is
> purposefully specified relative to the above path, rather than as
> absolute paths.

I think an approach where "/local/domain/$dm_domid/device-model/$domid/"
was in a local const char *root and then smth like:

   foreach foo(start_addr, size, name)
        foo = gcstrcat("/physmap/", foo);
	path = gcstrcat(root, foo)
	fooval = read(path)
	output(foo, fooval)

With the proper GC helpers etc of course. Would be fine, no?

The rel_path thing is only there because you are trying to keep "foo" and
"root" in the same string I think.

Ian.

Re: [PATCH for-4.6 5/8] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content

[Andrew Cooper]

On 31/07/15 17:56, Ian Campbell wrote:
> On Fri, 2015-07-31 at 17:38 +0100, Andrew Cooper wrote:
>> On 31/07/15 17:34, Ian Jackson wrote:
>>> Andrew Cooper writes ("Re: [PATCH for-4.6 5/8] tools/libxl: Save and 
>>> restore EMULATOR_XENSTORE_DATA content"):
>>>> On 29/07/15 12:49, Ian Jackson wrote:
>>>>>> +    rel_start = strlen(xs_path) - 7;
>>>>> This is horrible.
>>>> What do you recommend instead?
>>> I don't see why it is necessary to do something like rel_start at all.
>>>
>>> This whole patch could probably be made much simpler with something
>>> like
>>>
>>>    static const char *const physmap_entries[] = {
>>>        "start_addr", "size", "name", NULL
>>>    };
>>>
>>> and then loop over it a few times.
>> But the rel_path has nothing to do with which subkeys get chosen.
>>
>> It is to strip out the current domains information from
>> /local/domain/$dm_domid/device-model/$domid/.
>>
>> This is because both of the domid in the path will be different on the
>> other side of migration.  This is why EMULATOR_XENSTORE_DATA is
>> purposefully specified relative to the above path, rather than as
>> absolute paths.
> I think an approach where "/local/domain/$dm_domid/device-model/$domid/"
> was in a local const char *root and then smth like:
>
>    foreach foo(start_addr, size, name)
>         foo = gcstrcat("/physmap/", foo);
> 	path = gcstrcat(root, foo)
> 	fooval = read(path)
> 	output(foo, fooval)
>
> With the proper GC helpers etc of course. Would be fine, no?
>
> The rel_path thing is only there because you are trying to keep "foo" and
> "root" in the same string I think.

(With the leading '/' stripped out of foo)

rel_path is to extract foo from path in O(1), and does so without two
further memory allocations.

path necessarily needs to be absolute, and the relative root wont
change, key to key.

All of this code will change anyway because of other review.  Lets see
how it ends up after that.

~Andrew

[PATCH for-4.6 6/8] tools/libxl: Prepare to write multiple records with EMULATOR headers

[Andrew Cooper]

With the newly specified EMULATOR_XENSTORE_DATA record, there are two
libxl records with an emulator subheader.  Refactor the existing code to
make future additions easier.

* Calculate the subheader at stream start time, rather than on the fly.
  Its contents are not going to change.
* Introduce a new setup_emulator_write() to insert a sub header in the
  appropriate place before a blob of data.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_internal.h     |    3 +-
 tools/libxl/libxl_stream_write.c |  101 ++++++++++++++++++++++++++------------
 2 files changed, 72 insertions(+), 32 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 8426221..39a203c 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3016,10 +3016,11 @@ struct libxl__stream_write_state {
     libxl__datacopier_state dc;
     sws_record_done_cb record_done_callback;
 
-    /* Only used when constructing an EMULATOR record. */
+    /* Only used when constructing EMULATOR records. */
     libxl__datacopier_state emu_dc;
     libxl__carefd *emu_carefd;
     libxl__sr_rec_hdr emu_rec_hdr;
+    libxl__sr_emulator_hdr emu_sub_hdr;
     void *emu_body;
 };
 
diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
index 9e9c998..3e6d399 100644
--- a/tools/libxl/libxl_stream_write.c
+++ b/tools/libxl/libxl_stream_write.c
@@ -95,12 +95,14 @@ static void write_done(libxl__egc *egc,
                        libxl__datacopier_state *dc,
                        int rc, int onwrite, int errnoval);
 
-/* Helper to set up reading some data from the stream. */
-static void setup_write(libxl__egc *egc,
-                        libxl__stream_write_state *stream,
-                        const char *what,
-                        libxl__sr_rec_hdr *hdr, void *body,
-                        sws_record_done_cb cb)
+/* Generic helper to set up writing some data to the stream. */
+static void setup_generic_write(libxl__egc *egc,
+                                libxl__stream_write_state *stream,
+                                const char *what,
+                                libxl__sr_rec_hdr *hdr,
+                                libxl__sr_emulator_hdr *emu_hdr,
+                                void *body,
+                                sws_record_done_cb cb)
 {
     static const uint8_t zero_padding[1U << REC_ALIGN_ORDER] = { 0 };
 
@@ -120,13 +122,21 @@ static void setup_write(libxl__egc *egc,
     }
 
     size_t padsz = ROUNDUP(hdr->length, REC_ALIGN_ORDER) - hdr->length;
+    uint32_t length = hdr->length;
 
     /* Insert header */
     libxl__datacopier_prefixdata(egc, dc, hdr, sizeof(*hdr));
 
+    /* Optional emulator sub-header */
+    if (emu_hdr) {
+        assert(length >= sizeof(*emu_hdr));
+        libxl__datacopier_prefixdata(egc, dc, emu_hdr, sizeof(*emu_hdr));
+        length -= sizeof(*emu_hdr);
+    }
+
     /* Optional body */
     if (body)
-        libxl__datacopier_prefixdata(egc, dc, body, hdr->length);
+        libxl__datacopier_prefixdata(egc, dc, body, length);
 
     /* Any required padding */
     if (padsz > 0)
@@ -135,6 +145,30 @@ static void setup_write(libxl__egc *egc,
     stream->record_done_callback = cb;
 }
 
+/* Helper to set up writing a regular record to the stream. */
+static void setup_write(libxl__egc *egc,
+                        libxl__stream_write_state *stream,
+                        const char *what,
+                        libxl__sr_rec_hdr *hdr,
+                        void *body,
+                        sws_record_done_cb cb)
+{
+    setup_generic_write(egc, stream, what, hdr, NULL, body, cb);
+}
+
+/* Helper to set up writing a record with an emulator prefix to the stream. */
+static void setup_emulator_write(libxl__egc *egc,
+                                 libxl__stream_write_state *stream,
+                                 const char *what,
+                                 libxl__sr_rec_hdr *hdr,
+                                 libxl__sr_emulator_hdr *emu_hdr,
+                                 void *body,
+                                 sws_record_done_cb cb)
+{
+    setup_generic_write(egc, stream, what, hdr, emu_hdr, body, cb);
+}
+
+
 static void write_done(libxl__egc *egc,
                        libxl__datacopier_state *dc,
                        int rc, int onwrite, int errnoval)
@@ -164,6 +198,7 @@ void libxl__stream_write_init(libxl__stream_write_state *stream)
     FILLZERO(stream->emu_dc);
     stream->emu_carefd = NULL;
     FILLZERO(stream->emu_rec_hdr);
+    FILLZERO(stream->emu_sub_hdr);
     stream->emu_body = NULL;
 }
 
@@ -171,6 +206,7 @@ void libxl__stream_write_start(libxl__egc *egc,
                                libxl__stream_write_state *stream)
 {
     libxl__datacopier_state *dc = &stream->dc;
+    libxl__domain_suspend_state *dss = stream->dss;
     STATE_AO_GC(stream->ao);
     struct libxl__sr_hdr hdr;
     int rc = 0;
@@ -179,6 +215,24 @@ void libxl__stream_write_start(libxl__egc *egc,
 
     stream->running = true;
 
+    if (dss->type == LIBXL_DOMAIN_TYPE_HVM) {
+        switch(libxl__device_model_version_running(gc, dss->domid)) {
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+            stream->emu_sub_hdr.id = EMULATOR_QEMU_TRADITIONAL;
+            break;
+
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+            stream->emu_sub_hdr.id = EMULATOR_QEMU_UPSTREAM;
+            break;
+
+        default:
+            rc = ERROR_FAIL;
+            LOG(ERROR, "Unknown emulator for HVM domain\n");
+            goto err;
+        }
+        stream->emu_sub_hdr.index = 0;
+    }
+
     dc->ao        = ao;
     dc->readfd    = -1;
     dc->writewhat = "stream header";
@@ -342,7 +396,6 @@ static void write_emulator_record(libxl__egc *egc,
     libxl__datacopier_state *dc = &stream->emu_dc;
     STATE_AO_GC(stream->ao);
     struct libxl__sr_rec_hdr *rec = &stream->emu_rec_hdr;
-    struct libxl__sr_emulator_hdr *ehdr = NULL;
     struct stat st;
     int rc;
 
@@ -350,7 +403,6 @@ static void write_emulator_record(libxl__egc *egc,
 
     /* Convenience aliases */
     const char *const filename = dss->dm_savefile;
-    const uint32_t domid = dss->domid;
 
     libxl__carefd_begin();
     int readfd = open(filename, O_RDONLY);
@@ -374,23 +426,8 @@ static void write_emulator_record(libxl__egc *egc,
     }
 
     rec->type = REC_TYPE_EMULATOR_CONTEXT;
-    rec->length = st.st_size + sizeof(*ehdr);
-    stream->emu_body = ehdr = libxl__malloc(NOGC, rec->length);
-
-    switch(libxl__device_model_version_running(gc, domid)) {
-    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
-        ehdr->id = EMULATOR_QEMU_TRADITIONAL;
-        break;
-
-    case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
-        ehdr->id = EMULATOR_QEMU_UPSTREAM;
-        break;
-
-    default:
-        rc = ERROR_FAIL;
-        goto err;
-    }
-    ehdr->index = 0;
+    rec->length = st.st_size + sizeof(stream->emu_sub_hdr);
+    stream->emu_body = libxl__malloc(NOGC, st.st_size);
 
     FILLZERO(*dc);
     dc->ao            = stream->ao;
@@ -399,8 +436,8 @@ static void write_emulator_record(libxl__egc *egc,
     dc->readfd        = readfd;
     dc->writefd       = -1;
     dc->maxsz         = -1;
-    dc->readbuf       = stream->emu_body + sizeof(*ehdr);
-    dc->bytes_to_read = rec->length - sizeof(*ehdr);
+    dc->readbuf       = stream->emu_body;
+    dc->bytes_to_read = st.st_size;
     dc->callback      = emulator_read_done;
 
     rc = libxl__datacopier_start(dc);
@@ -429,9 +466,11 @@ static void emulator_read_done(libxl__egc *egc,
     libxl__carefd_close(stream->emu_carefd);
     stream->emu_carefd = NULL;
 
-    setup_write(egc, stream, "emulator record",
-                &stream->emu_rec_hdr, stream->emu_body,
-                emulator_record_done);
+    setup_emulator_write(egc, stream, "emulator record",
+                         &stream->emu_rec_hdr,
+                         &stream->emu_sub_hdr,
+                         stream->emu_body,
+                         emulator_record_done);
 }
 
 static void emulator_record_done(libxl__egc *egc,
-- 
1.7.10.4

Re: [PATCH for-4.6 6/8] tools/libxl: Prepare to write multiple records with EMULATOR headers

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.6 6/8] tools/libxl: Prepare to write multiple records with EMULATOR headers"):
> With the newly specified EMULATOR_XENSTORE_DATA record, there are two
> libxl records with an emulator subheader.  Refactor the existing code to
> make future additions easier.
...
> @@ -164,6 +198,7 @@ void libxl__stream_write_init(libxl__stream_write_state *stream)
>      FILLZERO(stream->emu_dc);
>      stream->emu_carefd = NULL;
>      FILLZERO(stream->emu_rec_hdr);
> +    FILLZERO(stream->emu_sub_hdr);

This this just cautionary ?

> +    if (dss->type == LIBXL_DOMAIN_TYPE_HVM) {
> +        switch(libxl__device_model_version_running(gc, dss->domid)) {

I know you are just moving this code, but: space needed after
`switch'.

Ian.

Re: [PATCH for-4.6 6/8] tools/libxl: Prepare to write multiple records with EMULATOR headers

[Andrew Cooper]

On 29/07/15 12:54, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH for-4.6 6/8] tools/libxl: Prepare to write multiple records with EMULATOR headers"):
>> With the newly specified EMULATOR_XENSTORE_DATA record, there are two
>> libxl records with an emulator subheader.  Refactor the existing code to
>> make future additions easier.
> ...
>> @@ -164,6 +198,7 @@ void libxl__stream_write_init(libxl__stream_write_state *stream)
>>      FILLZERO(stream->emu_dc);
>>      stream->emu_carefd = NULL;
>>      FILLZERO(stream->emu_rec_hdr);
>> +    FILLZERO(stream->emu_sub_hdr);
> This this just cautionary ?

Yes - the _init() function here initialises all private data.  Most of
it is probably not strictly necessary.

>
>> +    if (dss->type == LIBXL_DOMAIN_TYPE_HVM) {
>> +        switch(libxl__device_model_version_running(gc, dss->domid)) {
> I know you are just moving this code, but: space needed after
> `switch'.

So I do.

~Andrew

[PATCH for-4.6 7/8] libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records

[Andrew Cooper]

Read and write "toolstack" information using the new
EMULATOR_XENSTORE_DATA record, and have the conversion script take care
of the old format.

The entire libxc and libxl migration v2 streams are now bitness-neutral
in their records.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

Note: This is a binary incompatibility in the libxl migration stream over
the past few weeks, but we don't really care about maintaining
compatibility between development versions of Xen, and it does fix the
one remaining bitness issue in the stream.
---
 tools/libxl/libxl_stream_read.c            |   13 ++++--
 tools/libxl/libxl_stream_write.c           |   23 ++++++----
 tools/python/scripts/convert-legacy-stream |   66 +++++++++++++++++++++++++---
 3 files changed, 84 insertions(+), 18 deletions(-)

diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
index fd3675c..4231e44 100644
--- a/tools/libxl/libxl_stream_read.c
+++ b/tools/libxl/libxl_stream_read.c
@@ -533,9 +533,16 @@ static bool process_record(libxl__egc *egc,
         libxl__xc_domain_restore(egc, dcs, &stream->shs, 0, 0, 0);
         break;
 
-    case REC_TYPE_XENSTORE_DATA:
-        rc = libxl__toolstack_restore(dcs->guest_domid, rec->body,
-                                      rec->hdr.length, &stream->shs);
+    case REC_TYPE_EMULATOR_XENSTORE_DATA:
+        if (rec->hdr.length < sizeof(libxl__sr_emulator_hdr)) {
+            rc = ERROR_FAIL;
+            LOG(ERROR, "Emulator xenstore data record too short to contain header");
+            goto err;
+        }
+
+        rc = libxl__restore_emulator_xenstore_data(
+            dcs, rec->body + sizeof(libxl__sr_emulator_hdr),
+            rec->hdr.length - sizeof(libxl__sr_emulator_hdr));
         if (rc)
             goto err;
 
diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
index 3e6d399..f8956e3 100644
--- a/tools/libxl/libxl_stream_write.c
+++ b/tools/libxl/libxl_stream_write.c
@@ -350,20 +350,27 @@ static void write_toolstack_record(libxl__egc *egc,
     struct libxl__sr_rec_hdr rec;
     int rc;
     uint8_t *toolstack_buf = NULL; /* We must free this. */
-    uint32_t toolstack_len;
+    uint32_t toolstack_len = 0;
 
-    rc = libxl__toolstack_save(dss->domid, &toolstack_buf,
-                               &toolstack_len, dss);
+    rc = libxl__save_emulator_xenstore_data(dss, &toolstack_buf,
+                                            &toolstack_len);
     if (rc)
         goto err;
 
+    /* No record? - All done. */
+    if (toolstack_len == 0) {
+        toolstack_record_done(egc, stream);
+        free(toolstack_buf);
+        return;
+    }
+
     FILLZERO(rec);
-    rec.type = REC_TYPE_XENSTORE_DATA;
-    rec.length = toolstack_len;
+    rec.type = REC_TYPE_EMULATOR_XENSTORE_DATA;
+    rec.length = toolstack_len + sizeof(stream->emu_sub_hdr);
 
-    setup_write(egc, stream, "toolstack record",
-                &rec, toolstack_buf,
-                toolstack_record_done);
+    setup_emulator_write(egc, stream, "toolstack record",
+                         &rec, &stream->emu_sub_hdr, toolstack_buf,
+                         toolstack_record_done);
 
     free(toolstack_buf);
     return;
diff --git a/tools/python/scripts/convert-legacy-stream b/tools/python/scripts/convert-legacy-stream
index 16331a4..41fee10 100755
--- a/tools/python/scripts/convert-legacy-stream
+++ b/tools/python/scripts/convert-legacy-stream
@@ -70,7 +70,7 @@ class VM(object):
 
         # libxl
         self.libxl = fmt == "libxl"
-        self.xenstore = [] # Deferred "toolstack" records
+        self.emu_xenstore = "" # NUL terminated key&val pairs from "toolstack" records
 
 def write_libxc_ihdr():
     stream_write(pack(libxc.IHDR_FORMAT,
@@ -169,8 +169,10 @@ def write_libxl_end():
 def write_libxl_libxc_context():
     write_record(libxl.REC_TYPE_libxc_context, "")
 
-def write_libxl_xenstore_data(data):
-    write_record(libxl.REC_TYPE_xenstore_data, data)
+def write_libxl_emulator_xenstore_data(data):
+    write_record(libxl.REC_TYPE_emulator_xenstore_data,
+                 pack(libxl.EMULATOR_HEADER_FORMAT,
+                      libxl.EMULATOR_ID_unknown, 0) + data)
 
 def write_libxl_emulator_context(blob):
     write_record(libxl.REC_TYPE_emulator_context,
@@ -297,6 +299,57 @@ def read_pv_tail(vm):
     write_record(libxc.REC_TYPE_end, "")
 
 
+def read_libxl_toolstack(vm, data):
+
+    if len(data) < 8:
+        raise StreamError("Overly short libxl toolstack data")
+
+    ver, count = unpack("=II", data[:8])
+    data = data[8:]
+
+    if ver != 1:
+        raise StreamError("Cannot decode libxl toolstack version %u" % (ver, ))
+    info("    Version %u, count %u" % (ver, count))
+
+    for x in range(count):
+
+        if len(data) < 28:
+            raise StreamError("Remaining data too short for physmap header")
+
+        phys, start, size, namelen = unpack("=QQQI", data[:28])
+        data = data[28:]
+
+        if namelen == 0:
+            raise StreamError("No physmap info name")
+
+        # 64bit leaked 4 bytes of padding onto the end of name
+        if twidth == 64:
+            namelen += 4
+
+        if len(data) < namelen:
+            raise StreamError("Remaining data too short for physmap name")
+
+        name = data[:namelen]
+        data = data[namelen:]
+
+        # Strip padding off the end of name
+        if twidth == 64:
+            name = name[:-4]
+
+        if name[-1] != '\x00':
+            raise StreamError("physmap name not NUL terminated")
+
+        root = "physmap/%x" % (phys,)
+        kv = [root + "/start_addr", "%x" % (start, ),
+              root + "/size",       "%x" % (size, ),
+              root + "/name",       name[:-1]]
+
+        for key, val in zip(kv[0::2], kv[1::2]):
+            info("    '%s' = '%s'" % (key, val))
+
+        vm.emu_xenstore += '\x00'.join(kv) + '\x00'
+
+
 def read_chunks(vm):
 
     hvm_params = []
@@ -441,7 +494,7 @@ def read_chunks(vm):
                 info("  Toolstack Data: sz 0x%x" % (sz, ))
 
                 if vm.libxl:
-                    vm.xenstore.append(data)
+                    read_libxl_toolstack(vm, data)
                 else:
                     info("    Discarding")
 
@@ -544,9 +597,8 @@ def read_legacy_stream(vm):
         else:
             read_hvm_tail(vm)
 
-        if vm.libxl:
-            for rec in vm.xenstore:
-                write_libxl_xenstore_data(rec)
+        if vm.libxl and len(vm.emu_xenstore):
+            write_libxl_emulator_xenstore_data(vm.emu_xenstore)
 
         if not pv and (vm.libxl or qemu):
             read_qemu(vm)
-- 
1.7.10.4

Re: [PATCH for-4.6 7/8] libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records

[Wei Liu]

On Tue, Jul 28, 2015 at 10:44:42PM +0100, Andrew Cooper wrote:
> Read and write "toolstack" information using the new
> EMULATOR_XENSTORE_DATA record, and have the conversion script take care
> of the old format.
> 
> The entire libxc and libxl migration v2 streams are now bitness-neutral
> in their records.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> 
> Note: This is a binary incompatibility in the libxl migration stream over
> the past few weeks, but we don't really care about maintaining
> compatibility between development versions of Xen, and it does fix the
> one remaining bitness issue in the stream.

Makes sense. Making the stream bitness-neutral is definitely something
we want to do before releasing migration v2.

And the code looks good to me.

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH for-4.6 7/8] libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.6 7/8] libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records"):
> Read and write "toolstack" information using the new
> EMULATOR_XENSTORE_DATA record, and have the conversion script take care
> of the old format.
...
> +            LOG(ERROR, "Emulator xenstore data record too short to contai\
n header");

Wrap damage.  How about

  +            LOG(ERROR,
  + "Emulator xenstore data record too short to contain header");

or

  +            LOG(ERROR,
  +             "Emulator xenstore data record too short to contain header");

?

> +        rc = libxl__restore_emulator_xenstore_data(

( in wrong place.

But apart from that it looks fine.

Thanks,
Ian.

Re: [PATCH for-4.6 7/8] libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records

[Andrew Cooper]

On 29/07/15 13:00, Ian Jackson wrote:
>
>> +        rc = libxl__restore_emulator_xenstore_data(
> ( in wrong place.
>
> But apart from that it looks fine.

I am sorry, but this request is unreasonable IMO.

This is a function call, not an integer assignment.  Moving the bracket
onto the next line is counterintuitive, and I cannot find a single style
anywhere which suggests it being a rational thing to do.

~Andrew

Re: [PATCH for-4.6 7/8] libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [PATCH for-4.6 7/8] libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records"):
> On 29/07/15 13:00, Ian Jackson wrote:
> >> +        rc = libxl__restore_emulator_xenstore_data(
> > ( in wrong place.
> >
> > But apart from that it looks fine.
> 
> I am sorry, but this request is unreasonable IMO.
> 
> This is a function call, not an integer assignment.  Moving the bracket
> onto the next line is counterintuitive, and I cannot find a single style
> anywhere which suggests it being a rational thing to do.

I have done a search and AFAICT it is done the majority of the time,
by some considerable margin, in libxl.

Ian.

[PATCH for-4.6 8/8] tools/libxl: Drop all legacy "toolstack" record infrastructure

[Andrew Cooper]

No functional change.  It is not used any more.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_dom.c              |  223 ----------------------------------
 tools/libxl/libxl_internal.h         |    4 -
 tools/libxl/libxl_sr_stream_format.h |    1 -
 tools/python/xen/migration/libxl.py  |    1 -
 4 files changed, 229 deletions(-)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 885eb5e..67200a6 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1031,126 +1031,6 @@ int libxl__qemu_traditional_cmd(libxl__gc *gc, uint32_t domid,
     return libxl__xs_write(gc, XBT_NULL, path, "%s", cmd);
 }
 
-struct libxl__physmap_info {
-    uint64_t phys_offset;
-    uint64_t start_addr;
-    uint64_t size;
-    uint32_t namelen;
-    char name[];
-};
-
-/* Bump version every time when toolstack saved data changes.
- * Different types of data are arranged in the specified order.
- *
- * Version 1:
- *   uint32_t version
- *   QEMU physmap data:
- *     uint32_t count
- *     libxl__physmap_info * count
- */
-#define TOOLSTACK_SAVE_VERSION 1
-
-static inline char *restore_helper(libxl__gc *gc, uint32_t dm_domid,
-                                   uint32_t domid,
-                                   uint64_t phys_offset, char *node)
-{
-    return libxl__device_model_xs_path(gc, dm_domid, domid,
-                                       "/physmap/%"PRIx64"/%s",
-                                       phys_offset, node);
-}
-
-static int libxl__toolstack_restore_qemu(libxl__gc *gc, uint32_t domid,
-                                         const uint8_t *ptr, uint32_t size)
-{
-    int ret, i;
-    uint32_t count;
-    char *xs_path;
-    uint32_t dm_domid;
-    struct libxl__physmap_info *pi;
-
-    if (size < sizeof(count)) {
-        LOG(ERROR, "wrong size");
-        ret = -1;
-        goto out;
-    }
-
-    memcpy(&count, ptr, sizeof(count));
-    ptr += sizeof(count);
-
-    if (size < sizeof(count) + count*(sizeof(struct libxl__physmap_info))) {
-        LOG(ERROR, "wrong size");
-        ret = -1;
-        goto out;
-    }
-
-    dm_domid = libxl_get_stubdom_id(CTX, domid);
-    for (i = 0; i < count; i++) {
-        pi = (struct libxl__physmap_info*) ptr;
-        ptr += sizeof(struct libxl__physmap_info) + pi->namelen;
-
-        xs_path = restore_helper(gc, dm_domid, domid,
-                                 pi->phys_offset, "start_addr");
-        ret = libxl__xs_write(gc, 0, xs_path, "%"PRIx64, pi->start_addr);
-        if (ret) goto out;
-
-        xs_path = restore_helper(gc, dm_domid, domid, pi->phys_offset, "size");
-        ret = libxl__xs_write(gc, 0, xs_path, "%"PRIx64, pi->size);
-        if (ret) goto out;
-
-        if (pi->namelen > 0) {
-            xs_path = restore_helper(gc, dm_domid, domid,
-                                     pi->phys_offset, "name");
-            ret = libxl__xs_write(gc, 0, xs_path, "%s", pi->name);
-            if (ret) goto out;
-        }
-    }
-
-    ret = 0;
-out:
-    return ret;
-
-}
-
-static int libxl__toolstack_restore_v1(libxl__gc *gc, uint32_t domid,
-                                       const uint8_t *ptr, uint32_t size)
-{
-    return libxl__toolstack_restore_qemu(gc, domid, ptr, size);
-}
-
-int libxl__toolstack_restore(uint32_t domid, const uint8_t *ptr,
-                             uint32_t size, void *user)
-{
-    libxl__save_helper_state *shs = user;
-    libxl__domain_create_state *dcs = shs->caller_state;
-    STATE_AO_GC(dcs->ao);
-    int ret;
-    uint32_t version = 0, bufsize;
-
-    LOG(DEBUG,"domain=%"PRIu32" toolstack data size=%"PRIu32, domid, size);
-
-    if (size < sizeof(version)) {
-        LOG(ERROR, "wrong size");
-        ret = -1;
-        goto out;
-    }
-
-    memcpy(&version, ptr, sizeof(version));
-    ptr += sizeof(version);
-    bufsize = size - sizeof(version);
-
-    switch (version) {
-    case 1:
-        ret = libxl__toolstack_restore_v1(gc, domid, ptr, bufsize);
-        break;
-    default:
-        LOG(ERROR, "wrong version");
-        ret = -1;
-    }
-
-out:
-    return ret;
-}
-
 int libxl__restore_emulator_xenstore_data(libxl__domain_create_state *dcs,
                                           const char *ptr, uint32_t size)
 {
@@ -1443,109 +1323,6 @@ static void switch_logdirty_done(libxl__egc *egc,
 
 /*----- callbacks, called by xc_domain_save -----*/
 
-static inline char *physmap_path(libxl__gc *gc, uint32_t dm_domid,
-                                 uint32_t domid,
-                                 char *phys_offset, char *node)
-{
-    return libxl__device_model_xs_path(gc, dm_domid, domid,
-                                       "/physmap/%s/%s",
-                                       phys_offset, node);
-}
-
-int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
-        uint32_t *len, void *dss_void)
-{
-    libxl__domain_suspend_state *dss = dss_void;
-    int ret;
-    STATE_AO_GC(dss->ao);
-    int i = 0;
-    uint32_t version = TOOLSTACK_SAVE_VERSION;
-    uint8_t *ptr = NULL;
-
-    ret = -1;
-
-    /* Version number */
-    *len = sizeof(version);
-    *buf = calloc(1, *len);
-    if (*buf == NULL) goto out;
-    ptr = *buf;
-    memcpy(ptr, &version, sizeof(version));
-
-    /* QEMU physmap data */
-    {
-        char **entries = NULL, *xs_path;
-        struct libxl__physmap_info *pi;
-        uint32_t dm_domid;
-        char *start_addr = NULL, *size = NULL, *phys_offset = NULL;
-        char *name = NULL;
-        unsigned int num = 0;
-        uint32_t count = 0, namelen = 0;
-
-        dm_domid = libxl_get_stubdom_id(CTX, domid);
-
-        xs_path = libxl__device_model_xs_path(gc, dm_domid, domid,
-                                              "/physmap");
-        entries = libxl__xs_directory(gc, 0, xs_path, &num);
-        count = num;
-
-        *len += sizeof(count);
-        *buf = realloc(*buf, *len);
-        if (*buf == NULL) goto out;
-        ptr = *buf + sizeof(version);
-        memcpy(ptr, &count, sizeof(count));
-        ptr += sizeof(count);
-
-        for (i = 0; i < count; i++) {
-            unsigned long offset;
-            phys_offset = entries[i];
-            if (phys_offset == NULL) {
-                LOG(ERROR, "phys_offset %d is NULL", i);
-                goto out;
-            }
-
-            xs_path = physmap_path(gc, dm_domid, domid, phys_offset,
-                                   "start_addr");
-            start_addr = libxl__xs_read(gc, 0, xs_path);
-            if (start_addr == NULL) {
-                LOG(ERROR, "%s is NULL", xs_path);
-                goto out;
-            }
-
-            xs_path = physmap_path(gc, dm_domid, domid, phys_offset, "size");
-            size = libxl__xs_read(gc, 0, xs_path);
-            if (size == NULL) {
-                LOG(ERROR, "%s is NULL", xs_path);
-                goto out;
-            }
-
-            xs_path = physmap_path(gc, dm_domid, domid, phys_offset, "name");
-            name = libxl__xs_read(gc, 0, xs_path);
-            if (name == NULL)
-                namelen = 0;
-            else
-                namelen = strlen(name) + 1;
-            *len += namelen + sizeof(struct libxl__physmap_info);
-            offset = ptr - (*buf);
-            *buf = realloc(*buf, *len);
-            if (*buf == NULL) goto out;
-            ptr = (*buf) + offset;
-            pi = (struct libxl__physmap_info *) ptr;
-            pi->phys_offset = strtoll(phys_offset, NULL, 16);
-            pi->start_addr = strtoll(start_addr, NULL, 16);
-            pi->size = strtoll(size, NULL, 16);
-            pi->namelen = namelen;
-            memcpy(pi->name, name, namelen);
-            ptr += sizeof(struct libxl__physmap_info) + namelen;
-        }
-    }
-
-    LOG(DEBUG,"domain=%"PRIu32" toolstack data size=%"PRIu32, domid, *len);
-
-    ret = 0;
-out:
-    return ret;
-}
-
 int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
                                        uint8_t **callee_buf,
                                        uint32_t *callee_len)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 39a203c..80c714a 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1091,8 +1091,6 @@ _hidden int libxl__domain_rename(libxl__gc *gc, uint32_t domid,
                                  const char *old_name, const char *new_name,
                                  xs_transaction_t trans);
 
-_hidden int libxl__toolstack_restore(uint32_t domid, const uint8_t *buf,
-                                     uint32_t size, void *data);
 _hidden int libxl__domain_resume_device_model(libxl__gc *gc, uint32_t domid);
 
 _hidden const char *libxl__userdata_path(libxl__gc *gc, uint32_t domid,
@@ -3438,8 +3436,6 @@ void libxl__xc_domain_saverestore_async_callback_done(libxl__egc *egc,
 
 _hidden void libxl__domain_suspend_common_switch_qemu_logdirty
                                (int domid, unsigned int enable, void *data);
-_hidden int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
-        uint32_t *len, void *data);
 _hidden int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
                                                uint8_t **buf, uint32_t *len);
 _hidden int libxl__restore_emulator_xenstore_data(
diff --git a/tools/libxl/libxl_sr_stream_format.h b/tools/libxl/libxl_sr_stream_format.h
index 4c23367..54da360 100644
--- a/tools/libxl/libxl_sr_stream_format.h
+++ b/tools/libxl/libxl_sr_stream_format.h
@@ -33,7 +33,6 @@
 
 #define REC_TYPE_END                    0x00000000U
 #define REC_TYPE_LIBXC_CONTEXT          0x00000001U
-#define REC_TYPE_XENSTORE_DATA          0x00000002U /* TOOLSTACK COMPAT */
 #define REC_TYPE_EMULATOR_XENSTORE_DATA 0x00000002U
 #define REC_TYPE_EMULATOR_CONTEXT       0x00000003U
 #define REC_TYPE_CHECKPOINT_END         0x00000004U
diff --git a/tools/python/xen/migration/libxl.py b/tools/python/xen/migration/libxl.py
index 1a9ca87..fc0acf6 100644
--- a/tools/python/xen/migration/libxl.py
+++ b/tools/python/xen/migration/libxl.py
@@ -34,7 +34,6 @@ RH_FORMAT = "II"
 
 REC_TYPE_end                    = 0x00000000
 REC_TYPE_libxc_context          = 0x00000001
-REC_TYPE_xenstore_data          = 0x00000002 # TOOLSTACK COMPAT
 REC_TYPE_emulator_xenstore_data = 0x00000002
 REC_TYPE_emulator_context       = 0x00000003
 REC_TYPE_checkpoint_end         = 0x00000004
-- 
1.7.10.4

Re: [PATCH for-4.6 8/8] tools/libxl: Drop all legacy "toolstack" record infrastructure

[Wei Liu]

On Tue, Jul 28, 2015 at 10:44:43PM +0100, Andrew Cooper wrote:
> No functional change.  It is not used any more.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH for-4.6 8/8] tools/libxl: Drop all legacy "toolstack" record infrastructure

[Ian Jackson]

Andrew Cooper writes ("[PATCH for-4.6 8/8] tools/libxl: Drop all legacy "toolstack" record infrastructure"):
> No functional change.  It is not used any more.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Re: [PATCH for-4.6 0/8] Fix libxl TOOLSTACK records for migration v2

[Ian Campbell]

On Tue, 2015-07-28 at 22:44 +0100, Andrew Cooper wrote:
> 
> Andrew Cooper (8):
>   tools/libxl: Only continue stream operations if the stream is still in 
> progress
>   tools/libxl: Assert that libxl__ao_inprogress_gc() is not called with 
> NULL

I applied these two.