All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2
@ 2015-08-04 17:16 Andrew Cooper
  2015-08-04 17:16 ` [PATCH for-4.6 v3 1/6] tools/libxl: Make libxl__conversion_helper_abort() safe to use Andrew Cooper
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Andrew Cooper @ 2015-08-04 17:16 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu

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.

Only minor changes from v3.  Details in patches.

Summary of Modified/Acks:

Andrew Cooper (6):
A  tools/libxl: Make libxl__conversion_helper_abort() safe to use
 M docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA
 M tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content
A  tools/libxl: Prepare to write multiple records with EMULATOR headers
A  libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records
A  tools/libxl: Drop all legacy "toolstack" record infrastructure

 docs/specs/libxl-migration-stream.pandoc   |   82 +++++---
 tools/libxl/libxl_convert_callout.c        |    2 +
 tools/libxl/libxl_dom.c                    |  290 ++++++++++------------------
 tools/libxl/libxl_internal.h               |   15 +-
 tools/libxl/libxl_sr_stream_format.h       |   10 +-
 tools/libxl/libxl_stream_read.c            |   27 ++-
 tools/libxl/libxl_stream_write.c           |  194 ++++++++++++-------
 tools/python/Makefile                      |    1 +
 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, 464 insertions(+), 337 deletions(-)

--
1.7.10.4

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH for-4.6 v3 1/6] tools/libxl: Make libxl__conversion_helper_abort() safe to use
  2015-08-04 17:16 [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2 Andrew Cooper
@ 2015-08-04 17:16 ` Andrew Cooper
  2015-08-04 17:16 ` [PATCH for-4.6 v3 2/6] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA Andrew Cooper
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2015-08-04 17:16 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Wei Liu

Previously, in the case of an error causing a call to
libxl__conversion_helper_abort() on a stream without legacy conversion,
libxl would fall over a NULL pointer because chs->ao was not set up.

Arrange for all ->ao's to be set up at _init() time, by having each
_init() function assert that their caller has done the right thing.
While doing so, introduce a previously-missing save_helper_init() in
stream_read_init().

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

v2: Make sure ->ao is always valid
---
 tools/libxl/libxl_convert_callout.c |    2 ++
 tools/libxl/libxl_internal.h        |    4 ++--
 tools/libxl/libxl_stream_read.c     |   11 ++++++++---
 tools/libxl/libxl_stream_write.c    |    5 +++++
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_convert_callout.c b/tools/libxl/libxl_convert_callout.c
index 65b4df9..5e5678b 100644
--- a/tools/libxl/libxl_convert_callout.c
+++ b/tools/libxl/libxl_convert_callout.c
@@ -34,6 +34,8 @@ static void helper_done(libxl__egc *egc,
 
 void libxl__conversion_helper_init(libxl__conversion_helper_state *chs)
 {
+    assert(chs->ao);
+
     chs->v2_carefd = NULL;
     chs->rc = 0;
     libxl__ao_abortable_init(&chs->abrt);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 911de2d..22d6a26 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2948,9 +2948,9 @@ _hidden void libxl__remus_devices_commit(libxl__egc *egc,
 typedef struct libxl__conversion_helper_state libxl__conversion_helper_state;
 
 struct libxl__conversion_helper_state {
-    /* public */
+    /* Public - Must be filled by caller unless noted. */
     libxl__ao *ao;
-    int legacy_fd;
+    int legacy_fd;             /* fd to read the legacy stream from. */
     bool hvm;                  /* pv or hvm domain? */
     libxl__carefd *v2_carefd;  /* Filled by successful call to
                                 * libxl__convert_legacy_stream().  Caller
diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
index fd3675c..c555542 100644
--- a/tools/libxl/libxl_stream_read.c
+++ b/tools/libxl/libxl_stream_read.c
@@ -173,12 +173,18 @@ static void free_record(libxl__sr_record_buf *rec)
 
 void libxl__stream_read_init(libxl__stream_read_state *stream)
 {
+    assert(stream->ao);
+
+    stream->shs.ao = stream->ao;
+    libxl__save_helper_init(&stream->shs);
+
+    stream->chs.ao = stream->ao;
+    libxl__conversion_helper_init(&stream->chs);
+
     stream->rc = 0;
     stream->running = false;
     stream->in_checkpoint = false;
     stream->sync_teardown = false;
-    libxl__save_helper_init(&stream->shs);
-    libxl__conversion_helper_init(&stream->chs);
     FILLZERO(stream->dc);
     FILLZERO(stream->hdr);
     LIBXL_STAILQ_INIT(&stream->record_queue);
@@ -205,7 +211,6 @@ void libxl__stream_read_start(libxl__egc *egc,
         /* Convert the legacy stream. */
         libxl__conversion_helper_state *chs = &stream->chs;
 
-        chs->ao = stream->ao;
         chs->legacy_fd = stream->fd;
         chs->hvm =
             (stream->dcs->guest_config->b_info.type == LIBXL_DOMAIN_TYPE_HVM);
diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
index 9e9c998..be8f548 100644
--- a/tools/libxl/libxl_stream_write.c
+++ b/tools/libxl/libxl_stream_write.c
@@ -155,6 +155,11 @@ static void write_done(libxl__egc *egc,
 
 void libxl__stream_write_init(libxl__stream_write_state *stream)
 {
+    assert(stream->ao);
+
+    stream->shs.ao = stream->ao;
+    libxl__save_helper_init(&stream->shs);
+
     stream->rc = 0;
     stream->running = false;
     stream->in_checkpoint = false;
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH for-4.6 v3 2/6] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA
  2015-08-04 17:16 [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2 Andrew Cooper
  2015-08-04 17:16 ` [PATCH for-4.6 v3 1/6] tools/libxl: Make libxl__conversion_helper_abort() safe to use Andrew Cooper
@ 2015-08-04 17:16 ` Andrew Cooper
  2015-08-05  9:37   ` Ian Campbell
  2015-08-05  9:40   ` Wei Liu
  2015-08-04 17:16 ` [PATCH for-4.6 v3 3/6] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content Andrew Cooper
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 12+ messages in thread
From: Andrew Cooper @ 2015-08-04 17:16 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu

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>

v3:
 * Remove ambiguous statement concerning the emulator index.
 * Install verify-stream-v2 into our private bindir.  I have a cunning plan
   for some test cases, and it will be useful to installed in 4.6

v2:
 * More adjustments to the libxl spec.
 * Make the emulator id/index table common and move it up beside the
   record type/length table.
 * Be more specific about the format and encoding of the xenstore
   key/value data.
 * Add a note about the unspecified nature of the emulator context blob.

squash! docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA
---
 docs/specs/libxl-migration-stream.pandoc   |   82 ++++++++++++++++++++--------
 tools/libxl/libxl_sr_stream_format.h       |   11 ++--
 tools/python/Makefile                      |    1 +
 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 +-
 7 files changed, 155 insertions(+), 54 deletions(-)

diff --git a/docs/specs/libxl-migration-stream.pandoc b/docs/specs/libxl-migration-stream.pandoc
index cdec168..2c97d86 100644
--- a/docs/specs/libxl-migration-stream.pandoc
+++ b/docs/specs/libxl-migration-stream.pandoc
@@ -90,8 +90,8 @@ i386, x86_64, or arm host.
 \clearpage
 
 
-Records
-=======
+Record Overview
+===============
 
 A record has a record header, type specific data and a trailing footer.  If
 `length` is not a multiple of 8, the body is padded with zeroes to align the
@@ -113,7 +113,7 @@ type         0x00000000: END
 
              0x00000001: LIBXC_CONTEXT
 
-             0x00000002: XENSTORE_DATA
+             0x00000002: EMULATOR_XENSTORE_DATA
 
              0x00000003: EMULATOR_CONTEXT
 
@@ -135,6 +135,38 @@ padding      0 to 7 octets of zeros to pad the whole record to a multiple
 
 \clearpage
 
+Emulator Records
+----------------
+
+Several records are specifically for emulators, and have a common sub header.
+
+     0     1     2     3     4     5     6     7 octet
+    +------------------------+------------------------+
+    | emulator_id            | index                  |
+    +------------------------+------------------------+
+    | record specific data                            |
+    ...
+    +-------------------------------------------------+
+
+--------------------------------------------------------------------
+Field            Description
+------------     ---------------------------------------------------
+emulator_id      0x00000000: Unknown (In the case of a legacy stream)
+
+                 0x00000001: Qemu Traditional
+
+                 0x00000002: Qemu Upstream
+
+                 0x00000003 - 0xFFFFFFFF: Reserved for future emulators.
+
+index            Index of this emulator for the domain.
+--------------------------------------------------------------------
+
+\clearpage
+
+Records
+=======
+
 END
 ----
 
@@ -163,17 +195,33 @@ 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/value data are encoded as a packed sequence of (key, value)
+tuples.  Each (key, value) tuple is a packed pair of NUL terminated octets,
+conforming to xenstore protocol character encoding (keys strictly as
+alphanumeric ASCII and `-/_@`, values expected to be human-readable ASCII).
+
+Keys shall be relative to to the device models xenstore tree for the new
+domain.  At the time of writing, keys are relative to the path
+
+> `/local/domain/$dm_domid/device-model/$domid/`
+
+although this path is free to change moving forward, thus should not be
+assumed.
+
 EMULATOR\_CONTEXT
 ----------------
 
@@ -187,22 +235,8 @@ A context blob for a specific emulator associated with the domain.
     ...
     +-------------------------------------------------+
 
---------------------------------------------------------------------
-Field            Description
-------------     ---------------------------------------------------
-emulator_id      0x00000000: Unknown (In the case of a legacy stream)
-
-                 0x00000001: Qemu Traditional
-
-                 0x00000002: Qemu Upstream
-
-                 0x00000003 - 0xFFFFFFFF: Reserved for future emulators.
-
-index            Index of this emulator for the domain, if multiple
-                 emulators are in use.
-
-emulator_ctx     Emulator context blob.
---------------------------------------------------------------------
+The *emulator_ctx* is a binary blob interpreted by the emulator identified by
+*emulator_id*.  Its format is unspecified.
 
 CHECKPOINT\_END
 ---------------
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/Makefile b/tools/python/Makefile
index 8d2c7dd..0395e50 100644
--- a/tools/python/Makefile
+++ b/tools/python/Makefile
@@ -23,6 +23,7 @@ install:
 		$(PYTHON_PREFIX_ARG) --root="$(DESTDIR)" --force
 
 	$(INSTALL_PROG) scripts/convert-legacy-stream $(DESTDIR)$(LIBEXEC_BIN)
+	$(INSTALL_PROG) scripts/verify-stream-v2 $(DESTDIR)$(LIBEXEC_BIN)
 
 .PHONY: test
 test:
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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH for-4.6 v3 3/6] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content
  2015-08-04 17:16 [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2 Andrew Cooper
  2015-08-04 17:16 ` [PATCH for-4.6 v3 1/6] tools/libxl: Make libxl__conversion_helper_abort() safe to use Andrew Cooper
  2015-08-04 17:16 ` [PATCH for-4.6 v3 2/6] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA Andrew Cooper
@ 2015-08-04 17:16 ` Andrew Cooper
  2015-08-05  9:39   ` Ian Campbell
  2015-08-05  9:40   ` Wei Liu
  2015-08-04 17:16 ` [PATCH for-4.6 v3 4/6] tools/libxl: Prepare to write multiple records with EMULATOR headers Andrew Cooper
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 12+ messages in thread
From: Andrew Cooper @ 2015-08-04 17:16 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell, Wei Liu

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,
which reimplement the existing libxl__toolstack_{save,restore}() logic.

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>

v2:
 * Drop _ prefix from helper functions.
 * Changes to string handling per maintainer request.
v2:
 * Factor out pointer arithmatic into helper functions
---
 tools/libxl/libxl_dom.c      |  135 ++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_internal.h |    4 ++
 2 files changed, 139 insertions(+)

diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 5555fea..d54d892 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -1151,6 +1151,76 @@ int libxl__toolstack_restore(uint32_t domid, const uint8_t *ptr,
     return ret;
 }
 
+/*
+ * Inspect the buffer between start and end, and return a pointer to the
+ * character following the NUL terminator of start, or NULL if start is not
+ * terminated before end.
+ */
+static const char *next_string(const char *start, const char *end)
+{
+    if (start >= end) return NULL;
+
+    size_t total_len = end - start;
+    size_t len = strnlen(start, total_len);
+
+    if (len == total_len)
+        return NULL;
+    else
+        return start + len + 1;
+}
+
+int libxl__restore_emulator_xenstore_data(libxl__domain_create_state *dcs,
+                                          const char *ptr, uint32_t size)
+{
+    STATE_AO_GC(dcs->ao);
+    const char *next = ptr, *end = ptr + size, *key, *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 (next < end) {
+        key = next;
+        next = next_string(next, end);
+
+        /* Sanitise 'key'. */
+        if (!next) {
+            rc = ERROR_FAIL;
+            LOG(ERROR, "Key in xenstore data not NUL terminated");
+            goto out;
+        }
+        if (key[0] == '\0') {
+            rc = ERROR_FAIL;
+            LOG(ERROR, "empty key found in xenstore data");
+            goto out;
+        }
+        if (key[0] == '/') {
+            rc = ERROR_FAIL;
+            LOG(ERROR, "Key in xenstore data not relative");
+            goto out;
+        }
+
+        val = next;
+        next = next_string(next, end);
+
+        /* Sanitise 'val'. */
+        if (!next) {
+            rc = ERROR_FAIL;
+            LOG(ERROR, "Val in xenstore data not NUL terminated");
+            goto out;
+        }
+
+        libxl__xs_write(gc, XBT_NULL,
+                        GCSPRINTF("%s/%s", xs_root, key), "%s", val);
+    }
+
+    rc = 0;
+
+ out:
+    return rc;
+}
+
 /*==================== Domain suspend (save) ====================*/
 
 static void stream_done(libxl__egc *egc,
@@ -1487,6 +1557,71 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
     return ret;
 }
 
+/*
+ * Expand the buffer 'buf' of length 'len', to append 'str' including its NUL
+ * terminator.
+ */
+static void append_string(libxl__gc *gc, char **buf, uint32_t *len,
+                          const char *str)
+{
+    size_t extralen = strlen(str) + 1;
+    char *new = libxl__realloc(gc, *buf, *len + extralen);
+
+    *buf = new;
+    memcpy(new + *len, str, extralen);
+    *len += extralen;
+}
+
+int libxl__save_emulator_xenstore_data(libxl__domain_suspend_state *dss,
+                                       char **callee_buf,
+                                       uint32_t *callee_len)
+{
+    STATE_AO_GC(dss->ao);
+    const char *xs_root;
+    char **entries, *buf = NULL;
+    unsigned int nr_entries, i, j, len = 0;
+    int rc;
+
+    const uint32_t domid = dss->domid;
+    const uint32_t dm_domid = libxl_get_stubdom_id(CTX, domid);
+
+    xs_root = libxl__device_model_xs_path(gc, dm_domid, domid, "");
+
+    entries = libxl__xs_directory(gc, 0, GCSPRINTF("%s/physmap", xs_root),
+                                  &nr_entries);
+    if (!entries || nr_entries == 0) { rc = 0; goto out; }
+
+    for (i = 0; i < nr_entries; ++i) {
+        static const char *const physmap_subkeys[] = {
+            "start_addr", "size", "name"
+        };
+
+        for (j = 0; j < ARRAY_SIZE(physmap_subkeys); ++j) {
+            const char *key = GCSPRINTF("physmap/%s/%s",
+                                        entries[i], physmap_subkeys[j]);
+
+            const char *val =
+                libxl__xs_read(gc, XBT_NULL,
+                               GCSPRINTF("%s/%s", xs_root, key));
+
+            if (!val) { rc = ERROR_FAIL; goto out; }
+
+            append_string(gc, &buf, &len, key);
+            append_string(gc, &buf, &len, val);
+        }
+    }
+
+    rc = 0;
+
+ out:
+    if (!rc) {
+        *callee_buf = 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 22d6a26..da2a785 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,
+                                               char **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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH for-4.6 v3 4/6] tools/libxl: Prepare to write multiple records with EMULATOR headers
  2015-08-04 17:16 [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2 Andrew Cooper
                   ` (2 preceding siblings ...)
  2015-08-04 17:16 ` [PATCH for-4.6 v3 3/6] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content Andrew Cooper
@ 2015-08-04 17:16 ` Andrew Cooper
  2015-08-04 17:16 ` [PATCH for-4.6 v3 5/6] libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records Andrew Cooper
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2015-08-04 17:16 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Wei Liu

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, and rename some functions for consistency
with the new scheme.

* 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.
* Rename *toolstack_* to *emulator_xenstore_*
* Rename *emulator_* to *emulator_context_*

No functional change.

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

v2:
 * Extra renaming for consistency.
---
 tools/libxl/libxl_internal.h     |    3 +-
 tools/libxl/libxl_stream_write.c |  182 +++++++++++++++++++++++---------------
 2 files changed, 113 insertions(+), 72 deletions(-)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index da2a785..fdef45e 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 be8f548..55439dd 100644
--- a/tools/libxl/libxl_stream_write.c
+++ b/tools/libxl/libxl_stream_write.c
@@ -38,14 +38,16 @@
  * The main loop for a plain VM writes:
  *  - Stream header
  *  - Libxc record
- *  - Toolstack record
- *  - if (hvm), Qemu record
+ *  - (optional) Emulator xenstore record
+ *  - if (hvm)
+ *      - Emulator context record
  *  - End record
  *
  * For checkpointed stream, there is a second loop which is triggered by a
  * save-helper checkpoint callback.  It writes:
- *  - Toolstack record
- *  - if (hvm), Qemu record
+ *  - (optional) Emulator xenstore record
+ *  - if (hvm)
+ *      - Emulator context record
  *  - Checkpoint end record
  */
 
@@ -69,17 +71,17 @@ static void stream_header_done(libxl__egc *egc,
 static void libxc_header_done(libxl__egc *egc,
                               libxl__stream_write_state *stream);
 /* libxl__xc_domain_save_done() lives here, event-order wise. */
-static void write_toolstack_record(libxl__egc *egc,
-                                   libxl__stream_write_state *stream);
-static void toolstack_record_done(libxl__egc *egc,
-                                  libxl__stream_write_state *stream);
-static void write_emulator_record(libxl__egc *egc,
-                                  libxl__stream_write_state *stream);
-static void emulator_read_done(libxl__egc *egc,
-                               libxl__datacopier_state *dc,
-                               int rc, int onwrite, int errnoval);
-static void emulator_record_done(libxl__egc *egc,
-                                 libxl__stream_write_state *stream);
+static void write_emulator_xenstore_record(libxl__egc *egc,
+                                           libxl__stream_write_state *stream);
+static void emulator_xenstore_record_done(libxl__egc *egc,
+                                          libxl__stream_write_state *stream);
+static void write_emulator_context_record(libxl__egc *egc,
+                                          libxl__stream_write_state *stream);
+static void emulator_context_read_done(libxl__egc *egc,
+                                       libxl__datacopier_state *dc,
+                                       int rc, int onwrite, int errnoval);
+static void emulator_context_record_done(libxl__egc *egc,
+                                         libxl__stream_write_state *stream);
 static void write_end_record(libxl__egc *egc,
                              libxl__stream_write_state *stream);
 
@@ -95,12 +97,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 +124,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 +147,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)
@@ -169,6 +205,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;
 }
 
@@ -176,6 +213,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;
@@ -184,6 +222,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";
@@ -216,7 +272,7 @@ void libxl__stream_write_start_checkpoint(libxl__egc *egc,
     assert(!stream->in_checkpoint);
     stream->in_checkpoint = true;
 
-    write_toolstack_record(egc, stream);
+    write_emulator_xenstore_record(egc, stream);
 }
 
 void libxl__stream_write_abort(libxl__egc *egc,
@@ -290,48 +346,47 @@ void libxl__xc_domain_save_done(libxl__egc *egc, void *dss_void,
      * If the stream is not still alive, we must not continue any work.
      */
     if (libxl__stream_write_inuse(stream))
-        write_toolstack_record(egc, stream);
+        write_emulator_xenstore_record(egc, stream);
 }
 
-static void write_toolstack_record(libxl__egc *egc,
-                                   libxl__stream_write_state *stream)
+static void write_emulator_xenstore_record(libxl__egc *egc,
+                                           libxl__stream_write_state *stream)
 {
     libxl__domain_suspend_state *dss = stream->dss;
     STATE_AO_GC(stream->ao);
     struct libxl__sr_rec_hdr rec;
     int rc;
-    uint8_t *toolstack_buf = NULL; /* We must free this. */
-    uint32_t toolstack_len;
+    uint8_t *buf = NULL; /* We must free this. */
+    uint32_t len;
 
-    rc = libxl__toolstack_save(dss->domid, &toolstack_buf,
-                               &toolstack_len, dss);
+    rc = libxl__toolstack_save(dss->domid, &buf, &len, dss);
     if (rc)
         goto err;
 
     FILLZERO(rec);
     rec.type = REC_TYPE_XENSTORE_DATA;
-    rec.length = toolstack_len;
+    rec.length = len;
 
-    setup_write(egc, stream, "toolstack record",
-                &rec, toolstack_buf,
-                toolstack_record_done);
+    setup_write(egc, stream, "emulator xenstore record",
+                &rec, buf,
+                emulator_xenstore_record_done);
 
-    free(toolstack_buf);
+    free(buf);
     return;
 
  err:
     assert(rc);
-    free(toolstack_buf);
+    free(buf);
     stream_complete(egc, stream, rc);
 }
 
-static void toolstack_record_done(libxl__egc *egc,
-                                  libxl__stream_write_state *stream)
+static void emulator_xenstore_record_done(libxl__egc *egc,
+                                          libxl__stream_write_state *stream)
 {
     libxl__domain_suspend_state *dss = stream->dss;
 
     if (dss->type == LIBXL_DOMAIN_TYPE_HVM)
-        write_emulator_record(egc, stream);
+        write_emulator_context_record(egc, stream);
     else {
         if (stream->in_checkpoint)
             write_checkpoint_end_record(egc, stream);
@@ -340,14 +395,13 @@ static void toolstack_record_done(libxl__egc *egc,
     }
 }
 
-static void write_emulator_record(libxl__egc *egc,
-                                  libxl__stream_write_state *stream)
+static void write_emulator_context_record(libxl__egc *egc,
+                                          libxl__stream_write_state *stream)
 {
     libxl__domain_suspend_state *dss = stream->dss;
     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;
 
@@ -355,7 +409,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);
@@ -379,23 +432,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;
@@ -404,9 +442,9 @@ 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->callback      = emulator_read_done;
+    dc->readbuf       = stream->emu_body;
+    dc->bytes_to_read = st.st_size;
+    dc->callback      = emulator_context_read_done;
 
     rc = libxl__datacopier_start(dc);
     if (rc)
@@ -419,9 +457,9 @@ static void write_emulator_record(libxl__egc *egc,
     stream_complete(egc, stream, rc);
 }
 
-static void emulator_read_done(libxl__egc *egc,
-                               libxl__datacopier_state *dc,
-                               int rc, int onwrite, int errnoval)
+static void emulator_context_read_done(libxl__egc *egc,
+                                       libxl__datacopier_state *dc,
+                                       int rc, int onwrite, int errnoval)
 {
     libxl__stream_write_state *stream = CONTAINER_OF(dc, *stream, emu_dc);
     STATE_AO_GC(stream->ao);
@@ -434,13 +472,15 @@ 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_context_record_done);
 }
 
-static void emulator_record_done(libxl__egc *egc,
-                                 libxl__stream_write_state *stream)
+static void emulator_context_record_done(libxl__egc *egc,
+                                         libxl__stream_write_state *stream)
 {
     free(stream->emu_body);
     stream->emu_body = NULL;
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH for-4.6 v3 5/6] libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records
  2015-08-04 17:16 [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2 Andrew Cooper
                   ` (3 preceding siblings ...)
  2015-08-04 17:16 ` [PATCH for-4.6 v3 4/6] tools/libxl: Prepare to write multiple records with EMULATOR headers Andrew Cooper
@ 2015-08-04 17:16 ` Andrew Cooper
  2015-08-04 17:16 ` [PATCH for-4.6 v3 6/6] tools/libxl: Drop all legacy "toolstack" record infrastructure Andrew Cooper
  2015-08-05  9:58 ` [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2 Ian Campbell
  6 siblings, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2015-08-04 17:16 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell

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>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.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            |   16 +++++--
 tools/libxl/libxl_stream_write.c           |   25 ++++++-----
 tools/python/scripts/convert-legacy-stream |   66 +++++++++++++++++++++++++---
 3 files changed, 85 insertions(+), 22 deletions(-)

diff --git a/tools/libxl/libxl_stream_read.c b/tools/libxl/libxl_stream_read.c
index c555542..4ec29da 100644
--- a/tools/libxl/libxl_stream_read.c
+++ b/tools/libxl/libxl_stream_read.c
@@ -538,14 +538,22 @@ 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;
 
         /*
-         * libxl__toolstack_restore() is a synchronous function.
+         * libxl__restore_emulator_xenstore_data() is a synchronous function.
          * Request that our caller queues another action for us.
          */
         further_action_needed = true;
diff --git a/tools/libxl/libxl_stream_write.c b/tools/libxl/libxl_stream_write.c
index 55439dd..10a9e0f 100644
--- a/tools/libxl/libxl_stream_write.c
+++ b/tools/libxl/libxl_stream_write.c
@@ -356,27 +356,30 @@ static void write_emulator_xenstore_record(libxl__egc *egc,
     STATE_AO_GC(stream->ao);
     struct libxl__sr_rec_hdr rec;
     int rc;
-    uint8_t *buf = NULL; /* We must free this. */
-    uint32_t len;
+    char *buf = NULL;
+    uint32_t len = 0;
 
-    rc = libxl__toolstack_save(dss->domid, &buf, &len, dss);
+    rc = libxl__save_emulator_xenstore_data(dss, &buf, &len);
     if (rc)
         goto err;
 
-    FILLZERO(rec);
-    rec.type = REC_TYPE_XENSTORE_DATA;
-    rec.length = len;
+    /* No record? - All done. */
+    if (len == 0) {
+        emulator_xenstore_record_done(egc, stream);
+        return;
+    }
 
-    setup_write(egc, stream, "emulator xenstore record",
-                &rec, buf,
-                emulator_xenstore_record_done);
+    FILLZERO(rec);
+    rec.type = REC_TYPE_EMULATOR_XENSTORE_DATA;
+    rec.length = len + sizeof(stream->emu_sub_hdr);
 
-    free(buf);
+    setup_emulator_write(egc, stream, "emulator xenstore record",
+                         &rec, &stream->emu_sub_hdr, buf,
+                         emulator_xenstore_record_done);
     return;
 
  err:
     assert(rc);
-    free(buf);
     stream_complete(egc, stream, rc);
 }
 
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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH for-4.6 v3 6/6] tools/libxl: Drop all legacy "toolstack" record infrastructure
  2015-08-04 17:16 [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2 Andrew Cooper
                   ` (4 preceding siblings ...)
  2015-08-04 17:16 ` [PATCH for-4.6 v3 5/6] libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records Andrew Cooper
@ 2015-08-04 17:16 ` Andrew Cooper
  2015-08-05  9:58 ` [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2 Ian Campbell
  6 siblings, 0 replies; 12+ messages in thread
From: Andrew Cooper @ 2015-08-04 17:16 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Ian Campbell

No functional change.  It is not used any more.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
CC: Ian Campbell <Ian.Campbell@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 d54d892..e1f11a3 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;
-}
-
 /*
  * Inspect the buffer between start and end, and return a pointer to the
  * character following the NUL terminator of start, or NULL if start is not
@@ -1454,109 +1334,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;
-}
-
 /*
  * Expand the buffer 'buf' of length 'len', to append 'str' including its NUL
  * terminator.
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index fdef45e..ad2a090 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,
                                                char **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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH for-4.6 v3 2/6] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA
  2015-08-04 17:16 ` [PATCH for-4.6 v3 2/6] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA Andrew Cooper
@ 2015-08-05  9:37   ` Ian Campbell
  2015-08-05  9:40   ` Wei Liu
  1 sibling, 0 replies; 12+ messages in thread
From: Ian Campbell @ 2015-08-05  9:37 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel; +Cc: Wei Liu, Ian Jackson

On Tue, 2015-08-04 at 18:16 +0100, 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.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---

Acked-by: Ian Campbell <Ian.Campbell@citrix.com>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH for-4.6 v3 3/6] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content
  2015-08-04 17:16 ` [PATCH for-4.6 v3 3/6] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content Andrew Cooper
@ 2015-08-05  9:39   ` Ian Campbell
  2015-08-05  9:40   ` Wei Liu
  1 sibling, 0 replies; 12+ messages in thread
From: Ian Campbell @ 2015-08-05  9:39 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel; +Cc: Wei Liu, Ian Jackson

On Tue, 2015-08-04 at 18:16 +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,
> which reimplement the existing libxl__toolstack_{save,restore}() logic.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---

Acked-by: Ian Campbell <Ian.Campbell@citrix.com>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH for-4.6 v3 2/6] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA
  2015-08-04 17:16 ` [PATCH for-4.6 v3 2/6] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA Andrew Cooper
  2015-08-05  9:37   ` Ian Campbell
@ 2015-08-05  9:40   ` Wei Liu
  1 sibling, 0 replies; 12+ messages in thread
From: Wei Liu @ 2015-08-05  9:40 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Ian Jackson, Ian Campbell, Xen-devel

On Tue, Aug 04, 2015 at 06:16:32PM +0100, 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.
> 
> 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>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH for-4.6 v3 3/6] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content
  2015-08-04 17:16 ` [PATCH for-4.6 v3 3/6] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content Andrew Cooper
  2015-08-05  9:39   ` Ian Campbell
@ 2015-08-05  9:40   ` Wei Liu
  1 sibling, 0 replies; 12+ messages in thread
From: Wei Liu @ 2015-08-05  9:40 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Ian Jackson, Ian Campbell, Xen-devel

On Tue, Aug 04, 2015 at 06:16:33PM +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,
> which reimplement the existing libxl__toolstack_{save,restore}() logic.
> 
> 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>

> 
> v2:

  v3  :-)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2
  2015-08-04 17:16 [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2 Andrew Cooper
                   ` (5 preceding siblings ...)
  2015-08-04 17:16 ` [PATCH for-4.6 v3 6/6] tools/libxl: Drop all legacy "toolstack" record infrastructure Andrew Cooper
@ 2015-08-05  9:58 ` Ian Campbell
  6 siblings, 0 replies; 12+ messages in thread
From: Ian Campbell @ 2015-08-05  9:58 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel; +Cc: Wei Liu, Ian Jackson

On Tue, 2015-08-04 at 18:16 +0100, Andrew Cooper wrote:
> 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.
> 
> Only minor changes from v3.  Details in patches.
> 
> Summary of Modified/Acks:
> 
> Andrew Cooper (6):
> A  tools/libxl: Make libxl__conversion_helper_abort() safe to use
>  M docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA
>  M tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content

I acked these two and applied, thanks!

(I don't recall if I saw a formal release-ack for these, but it's on the
blocker list so I didn't bother looking...)

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2015-08-05  9:58 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-04 17:16 [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2 Andrew Cooper
2015-08-04 17:16 ` [PATCH for-4.6 v3 1/6] tools/libxl: Make libxl__conversion_helper_abort() safe to use Andrew Cooper
2015-08-04 17:16 ` [PATCH for-4.6 v3 2/6] docs/libxl: Re-specify XENSTORE_DATA as EMULATOR_XENSTORE_DATA Andrew Cooper
2015-08-05  9:37   ` Ian Campbell
2015-08-05  9:40   ` Wei Liu
2015-08-04 17:16 ` [PATCH for-4.6 v3 3/6] tools/libxl: Save and restore EMULATOR_XENSTORE_DATA content Andrew Cooper
2015-08-05  9:39   ` Ian Campbell
2015-08-05  9:40   ` Wei Liu
2015-08-04 17:16 ` [PATCH for-4.6 v3 4/6] tools/libxl: Prepare to write multiple records with EMULATOR headers Andrew Cooper
2015-08-04 17:16 ` [PATCH for-4.6 v3 5/6] libxl/save&restore&convert: Switch to new EMULATOR_XENSTORE_DATA records Andrew Cooper
2015-08-04 17:16 ` [PATCH for-4.6 v3 6/6] tools/libxl: Drop all legacy "toolstack" record infrastructure Andrew Cooper
2015-08-05  9:58 ` [PATCH for-4.6 v3 0/6] Fix libxl TOOLSTACK records for migration v2 Ian Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.