[v2,03/17] tools/migration: Drop IHDR_VERSION constant from libxc and python
diff mbox series

Message ID 20200127143444.25538-4-andrew.cooper3@citrix.com
State New
Headers show
Series
  • Support CPUID/MSR data in migration streams
Related show

Commit Message

Andrew Cooper Jan. 27, 2020, 2:34 p.m. UTC
Migration v3 is in the process of being introduced, meaning that the code has
to cope with both versions.  Use an explicit 2 for now.

For the verify-stream-v2 and convert-legacy-stream scripts, update text to say
"v2 (or later)".  What matters is the distinction vs legacy streams.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 tools/libxc/xc_sr_restore.c                | 6 +++---
 tools/libxc/xc_sr_save.c                   | 2 +-
 tools/libxc/xc_sr_stream_format.h          | 1 -
 tools/python/scripts/convert-legacy-stream | 6 +++---
 tools/python/scripts/verify-stream-v2      | 2 +-
 tools/python/xen/migration/libxc.py        | 9 ++++-----
 6 files changed, 12 insertions(+), 14 deletions(-)

Comments

Ian Jackson Feb. 24, 2020, 5:25 p.m. UTC | #1
Andrew Cooper writes ("[PATCH v2 03/17] tools/migration: Drop IHDR_VERSION constant from libxc and python"):
> Migration v3 is in the process of being introduced, meaning that the code has
> to cope with both versions.  Use an explicit 2 for now.
> 
> For the verify-stream-v2 and convert-legacy-stream scripts, update
> text to say "v2 (or later)".  What matters is the distinction vs
> legacy streams.

Can I request that you use a manifest constant for `2', rather than
sprinkling literal `2's everywhere ?  Something like IHDR_VERSION_2 ?
This may seem pointless, but it will mean that it is possible to grep
the code much more easily for things which are relevant to v2 or v3 or
whatever.

(I don't it's necessary to go to any great lengths to substitute the
value of IHDR_VERSION_2 into error messages; a literal 2 in the string
is OK I think.)

Thanks,
Ian.
Andrew Cooper Feb. 24, 2020, 5:32 p.m. UTC | #2
On 24/02/2020 17:25, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH v2 03/17] tools/migration: Drop IHDR_VERSION constant from libxc and python"):
>> Migration v3 is in the process of being introduced, meaning that the code has
>> to cope with both versions.  Use an explicit 2 for now.
>>
>> For the verify-stream-v2 and convert-legacy-stream scripts, update
>> text to say "v2 (or later)".  What matters is the distinction vs
>> legacy streams.
> Can I request that you use a manifest constant for `2', rather than
> sprinkling literal `2's everywhere ?  Something like IHDR_VERSION_2 ?
> This may seem pointless, but it will mean that it is possible to grep
> the code much more easily for things which are relevant to v2 or v3 or
> whatever.
>
> (I don't it's necessary to go to any great lengths to substitute the
> value of IHDR_VERSION_2 into error messages; a literal 2 in the string
> is OK I think.)

As I reply previously... The value comes out of a pipe, and is checked
exactly once.

You can already grep for everything, using format_version which is where
this number is stashed for the duration of restore.

~Andrew
Ian Jackson March 5, 2020, 5:11 p.m. UTC | #3
Andrew Cooper writes ("Re: [PATCH v2 03/17] tools/migration: Drop IHDR_VERSION constant from libxc and python"):
> On 24/02/2020 17:25, Ian Jackson wrote:
> > Andrew Cooper writes ("[PATCH v2 03/17] tools/migration: Drop IHDR_VERSION constant from libxc and python"):
> >> Migration v3 is in the process of being introduced, meaning that the code has
> >> to cope with both versions.  Use an explicit 2 for now.
> >>
> >> For the verify-stream-v2 and convert-legacy-stream scripts, update
> >> text to say "v2 (or later)".  What matters is the distinction vs
> >> legacy streams.
> > Can I request that you use a manifest constant for `2', rather than
> > sprinkling literal `2's everywhere ?  Something like IHDR_VERSION_2 ?
> > This may seem pointless, but it will mean that it is possible to grep
> > the code much more easily for things which are relevant to v2 or v3 or
> > whatever.
> >
> > (I don't it's necessary to go to any great lengths to substitute the
> > value of IHDR_VERSION_2 into error messages; a literal 2 in the string
> > is OK I think.)
> 
> As I reply previously... The value comes out of a pipe, and is checked
> exactly once.

I think we are talking at cross purposes.

I am suggesting that you replace every instance of a numeric literal
`2' in the code with IHDR_VERSION_2 (which would be a #define or enum
for 2).

I count 4 such literals.

> You can already grep for everything, using format_version which is where
> this number is stashed for the duration of restore.

None of the things I am talking about have "format_version" nearby.
They tend to have variants on "version" but that is a poor thing to
grep for.

Am I making any more sense now ?

Thanks,
Ian.
Andrew Cooper April 14, 2020, 6:16 p.m. UTC | #4
On 05/03/2020 17:11, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH v2 03/17] tools/migration: Drop IHDR_VERSION constant from libxc and python"):
>> On 24/02/2020 17:25, Ian Jackson wrote:
>>> Andrew Cooper writes ("[PATCH v2 03/17] tools/migration: Drop IHDR_VERSION constant from libxc and python"):
>>>> Migration v3 is in the process of being introduced, meaning that the code has
>>>> to cope with both versions.  Use an explicit 2 for now.
>>>>
>>>> For the verify-stream-v2 and convert-legacy-stream scripts, update
>>>> text to say "v2 (or later)".  What matters is the distinction vs
>>>> legacy streams.
>>> Can I request that you use a manifest constant for `2', rather than
>>> sprinkling literal `2's everywhere ?  Something like IHDR_VERSION_2 ?
>>> This may seem pointless, but it will mean that it is possible to grep
>>> the code much more easily for things which are relevant to v2 or v3 or
>>> whatever.
>>>
>>> (I don't it's necessary to go to any great lengths to substitute the
>>> value of IHDR_VERSION_2 into error messages; a literal 2 in the string
>>> is OK I think.)
>> As I reply previously... The value comes out of a pipe, and is checked
>> exactly once.
> I think we are talking at cross purposes.
>
> I am suggesting that you replace every instance of a numeric literal
> `2' in the code with IHDR_VERSION_2 (which would be a #define or enum
> for 2).
>
> I count 4 such literals.

I presume you mean here 2x send IHDR and 2x receive IHDR, one C and one
Python in each case.

I understand what you're suggesting.  I completely disagree with it, as
it obfuscates the logic and introduces a source of bugs for zero gain.

IHDR_VERSION_* isn't grepable.  You've got to find the only instance of
it in code to figure out what to search for.

>> You can already grep for everything, using format_version which is where
>> this number is stashed for the duration of restore.
> None of the things I am talking about have "format_version" nearby.
> They tend to have variants on "version" but that is a poor thing to
> grep for.

"ctx->restore.format_version = ihdr.version;" (the only instance in the
codebase at this point in the series) is immediately after ihdr is
sanity checked, which is sole time where idhr.version has its value
checked (in the restore path.  There is also exactly one place in the
save side, and any more than this would be buggy code).

The only thing IHDR_VERSION_* usefully gets you is the ability to get
the defines accidentally wrong, and introduce bugs that way.

~Andrew
Ian Jackson April 27, 2020, 4:07 p.m. UTC | #5
Andrew Cooper writes ("Re: [PATCH v2 03/17] tools/migration: Drop IHDR_VERSION constant from libxc and python"):
> I presume you mean here 2x send IHDR and 2x receive IHDR, one C and one
> Python in each case.
> 
> I understand what you're suggesting.  I completely disagree with it, as
> it obfuscates the logic and introduces a source of bugs for zero gain.
...
> The only thing IHDR_VERSION_* usefully gets you is the ability to get
> the defines accidentally wrong, and introduce bugs that way.

Andrew Cooper writes ("Re: [PATCH v2 07/17] libxc/restore: STATIC_DATA_END inference for v2 compatibility"):
> On 05/03/2020 16:24, Ian Jackson wrote:
> > You could handle that with a small bit of code around one of the call
> > sites to adjust the error handling.  (Also, what a mess, but I guess
> > we're here now...)
> 
> ... which is the majority the code you're trying to abstract away.
...
> tl;dr I could put an #ifdef x86'd static inline in the root common
> header (xc_sr_common.h), but the overall complexity is greater than what
> is presented here.

I still don't agree with you I'm afraid.  I went back through our
messages, and looked at the code again, and I think we are probably
still not communicating well.  However, I thought about how best to
deal with this disagreement.  As the actual author of much of this
code, and certainly the person putting a lot of effort in, you should
get quite a bit of leeway.

I considered taking your branch and showing you what I meant in code
terms.  But ultimately I think this is a waste of our time and I don't
want us to get into a pointless argument.  I don't think these issues
matter enough to be worth the debate.  I don't think there are actual
bugs here - we're talking about matters of style and taste.

So,

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

It would probably have been better for me to have got to this point
earlier.

Sorry,
Ian.

Patch
diff mbox series

diff --git a/tools/libxc/xc_sr_restore.c b/tools/libxc/xc_sr_restore.c
index 5e31908ca8..dc2ffcf855 100644
--- a/tools/libxc/xc_sr_restore.c
+++ b/tools/libxc/xc_sr_restore.c
@@ -35,10 +35,10 @@  static int read_headers(struct xc_sr_context *ctx)
         return -1;
     }
 
-    if ( ihdr.version != IHDR_VERSION )
+    if ( ihdr.version != 2 )
     {
-        ERROR("Invalid Version: Expected %d, Got %d",
-              IHDR_VERSION, ihdr.version);
+        ERROR("Invalid Version: Expected 2, Got %d",
+              ihdr.version);
         return -1;
     }
 
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index fa736a311f..5c5ce18ac3 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -13,7 +13,7 @@  static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type)
     struct xc_sr_ihdr ihdr = {
         .marker  = IHDR_MARKER,
         .id      = htonl(IHDR_ID),
-        .version = htonl(IHDR_VERSION),
+        .version = htonl(2),
         .options = htons(IHDR_OPT_LITTLE_ENDIAN),
     };
     struct xc_sr_dhdr dhdr = {
diff --git a/tools/libxc/xc_sr_stream_format.h b/tools/libxc/xc_sr_stream_format.h
index 37a7da6eab..ae7c0de393 100644
--- a/tools/libxc/xc_sr_stream_format.h
+++ b/tools/libxc/xc_sr_stream_format.h
@@ -23,7 +23,6 @@  struct xc_sr_ihdr
 
 #define IHDR_MARKER  0xffffffffffffffffULL
 #define IHDR_ID      0x58454E46U
-#define IHDR_VERSION 2
 
 #define _IHDR_OPT_ENDIAN 0
 #define IHDR_OPT_LITTLE_ENDIAN (0 << _IHDR_OPT_ENDIAN)
diff --git a/tools/python/scripts/convert-legacy-stream b/tools/python/scripts/convert-legacy-stream
index 2922fb3185..02a194178f 100755
--- a/tools/python/scripts/convert-legacy-stream
+++ b/tools/python/scripts/convert-legacy-stream
@@ -79,7 +79,7 @@  def write_libxc_ihdr():
     stream_write(pack(libxc.IHDR_FORMAT,
                       libxc.IHDR_MARKER,  # Marker
                       libxc.IHDR_IDENT,   # Ident
-                      libxc.IHDR_VERSION, # Version
+                      2,                  # Version
                       libxc.IHDR_OPT_LE,  # Options
                       0, 0))              # Reserved
 
@@ -632,13 +632,13 @@  def main():
                           usage = ("%prog [options] -i INPUT -o OUTPUT"
                                    " -w WIDTH -g GUEST"),
                           description =
-                          "Convert a legacy stream to a v2 stream")
+                          "Convert a legacy stream to a v2 (or later) stream")
 
     # Required options
     parser.add_option("-i", "--in", dest = "fin", metavar = "<FD or FILE>",
                       help = "Legacy input to convert")
     parser.add_option("-o", "--out", dest = "fout", metavar = "<FD or FILE>",
-                      help = "v2 format output")
+                      help = "v2 (or later) format output")
     parser.add_option("-w", "--width", dest = "twidth",
                       metavar = "<32/64>", choices = ["32", "64"],
                       help = "Legacy toolstack bitness")
diff --git a/tools/python/scripts/verify-stream-v2 b/tools/python/scripts/verify-stream-v2
index 8bac04d566..fe82b86c11 100755
--- a/tools/python/scripts/verify-stream-v2
+++ b/tools/python/scripts/verify-stream-v2
@@ -108,7 +108,7 @@  def main():
 
     parser = OptionParser(usage = "%prog [options]",
                           description =
-                          "Verify a stream according to the v2 spec")
+                          "Verify a stream according to the v2 (or later) spec")
 
     # Optional options
     parser.add_option("-i", "--in", dest = "fin", metavar = "<FD or FILE>",
diff --git a/tools/python/xen/migration/libxc.py b/tools/python/xen/migration/libxc.py
index 8a800df980..63b3558029 100644
--- a/tools/python/xen/migration/libxc.py
+++ b/tools/python/xen/migration/libxc.py
@@ -19,7 +19,6 @@ 
 
 IHDR_MARKER  = 0xffffffffffffffff
 IHDR_IDENT   = 0x58454E46 # "XENF" in ASCII
-IHDR_VERSION = 2
 
 IHDR_OPT_BIT_ENDIAN = 0
 IHDR_OPT_LE = (0 << IHDR_OPT_BIT_ENDIAN)
@@ -113,7 +112,7 @@ 
 HVM_PARAMS_FORMAT         = "II"
 
 class VerifyLibxc(VerifyBase):
-    """ Verify a Libxc v2 stream """
+    """ Verify a Libxc v2 (or later) stream """
 
     def __init__(self, info, read):
         VerifyBase.__init__(self, info, read)
@@ -144,9 +143,9 @@  def verify_ihdr(self):
             raise StreamError("Bad image id: Expected 0x%x, got 0x%x" %
                               (IHDR_IDENT, ident))
 
-        if version != IHDR_VERSION:
-            raise StreamError("Unknown image version: Expected %d, got %d" %
-                              (IHDR_VERSION, version))
+        if version != 2:
+            raise StreamError("Unknown image version: Expected 2, got %d" %
+                              (version, ))
 
         if options & IHDR_OPT_RESZ_MASK:
             raise StreamError("Reserved bits set in image options field: 0x%x" %