> On Tue, 2018-10-09 at 17:24 +0100, Frediano Ziglio wrote:
> > The idea in version 1 of the protocol was to extend it using the
> > minor version.
> > However this was replaced by the usage of capabilities and the minor
> > attribute (which was not much used in version 1) was abandoned in
> > version 2.
> > This patch create a big difference in the code generated but only
> > because the minor version was passed between all possible functions.
> > Note that external function, for compatibility retains the minor
> > argument.
>
> What do you mean by "external function" here? Can you be more explicit?
I think a better adjective is "exported", some function pointers are returned
back, these function retain the same signature.
Does:
"Note that exported functions, for compatibility retains the minor
argument to avoid changing their signature."
sounds fine?
> One more comment below.
>
>
> >
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> > docs/spice_protocol.txt | 5 ----
> > python_modules/demarshal.py | 59 +++++++--------------------------
> > ----
> > python_modules/ptypes.py | 52 --------------------------------
> > 3 files changed, 10 insertions(+), 106 deletions(-)
> >
> > diff --git a/docs/spice_protocol.txt b/docs/spice_protocol.txt
> > index 421393d..b205743 100644
> > --- a/docs/spice_protocol.txt
> > +++ b/docs/spice_protocol.txt
> > @@ -445,11 +445,6 @@ zero
> >
> > TODO
> >
> > -minor
> > -~~~~~
> > -
> > -TODO
> > -
> > virtual
> > ~~~~~~~
> >
> > diff --git a/python_modules/demarshal.py
> > b/python_modules/demarshal.py
> > index 5209272..6fe3c66 100644
> > --- a/python_modules/demarshal.py
> > +++ b/python_modules/demarshal.py
> > @@ -92,8 +92,8 @@ def write_parser_helpers(writer):
> > writer.newline()
> > writer.statement("typedef struct PointerInfo PointerInfo")
> > writer.statement("typedef void (*message_destructor_t)(uint8_t
> > *message)")
> > - writer.statement("typedef uint8_t * (*parse_func_t)(uint8_t
> > *message_start, uint8_t *message_end, uint8_t *struct_data,
> > PointerInfo *ptr_info, int minor)")
> > - writer.statement("typedef uint8_t * (*parse_msg_func_t)(uint8_t
> > *message_start, uint8_t *message_end, int minor, size_t *size_out,
> > message_destructor_t *free_message)")
> > + writer.statement("typedef uint8_t * (*parse_func_t)(uint8_t
> > *message_start, uint8_t *message_end, uint8_t *struct_data,
> > PointerInfo *ptr_info)")
> > + writer.statement("typedef uint8_t * (*parse_msg_func_t)(uint8_t
> > *message_start, uint8_t *message_end, size_t *size_out,
> > message_destructor_t *free_message)")
> > writer.statement("typedef uint8_t *
> > (*spice_parse_channel_func_t)(uint8_t *message_start, uint8_t
> > *message_end, uint16_t message_type, int minor, size_t *size_out,
> > message_destructor_t *free_message)")
> >
> > writer.newline()
> > @@ -216,7 +216,7 @@ def write_validate_struct_function(writer,
> > struct):
> >
> > writer.set_is_generated("validator", validate_function)
> > writer = writer.function_helper()
> > - scope = writer.function(validate_function, "static intptr_t",
> > "uint8_t *message_start, uint8_t *message_end, uint64_t offset,
> > SPICE_GNUC_UNUSED int minor")
> > + scope = writer.function(validate_function, "static intptr_t",
> > "uint8_t *message_start, uint8_t *message_end, uint64_t offset")
> > scope.variable_def("uint8_t *", "start = message_start +
> > offset")
> > scope.variable_def("SPICE_GNUC_UNUSED uint8_t *", "pos")
> > scope.variable_def("uint64_t", "mem_size", "nw_size")
> > @@ -301,7 +301,7 @@ def write_validate_pointer_item(writer,
> > container, item, scope, parent_scope, st
> >
> > elif target_type.is_struct():
> > validate_function =
> > write_validate_struct_function(writer, target_type)
> > - writer.assign("ptr_size", "%s(message_start,
> > message_end, %s, minor)" % (validate_function, v))
> > + writer.assign("ptr_size", "%s(message_start,
> > message_end, %s)" % (validate_function, v))
> > writer.error_check("ptr_size < 0")
> >
> > if want_extra_size:
> > @@ -512,12 +512,8 @@ def write_validate_member(writer, mprefix,
> > container, member, parent_scope, star
> > if member.has_attr("virtual"):
> > return
> >
> > - if member.has_minor_attr():
> > - prefix = "if (minor >= %s)" % (member.get_minor_attr())
> > - newline = False
> > - else:
> > - prefix = ""
> > - newline = True
> > + prefix = ""
> > + newline = True
> > item = MemberItemInfo(member, mprefix, container, start)
> > with writer.block(prefix, newline=newline, comment=member.name)
> > as scope:
> > if member.is_switch():
> > @@ -527,24 +523,6 @@ def write_validate_member(writer, mprefix,
> > container, member, parent_scope, star
> > write_validate_item(writer, container, item, scope,
> > parent_scope, start,
> > want_nw_size, want_mem_size,
> > want_extra_size)
> >
> > - if member.has_minor_attr():
> > - with writer.block(" else", comment = "minor < %s" %
> > (member.get_minor_attr())):
> > - if member.is_array():
> > - nelements = "%s__nelements" %(item.prefix)
> > - writer.assign(nelements, 0)
> > - if want_nw_size:
> > - writer.assign(item.nw_size(), 0)
> > -
> > - if want_mem_size:
> > - if member.is_fixed_sizeof():
> > - writer.assign(item.mem_size(), member.sizeof())
> > - elif member.is_array():
> > - writer.assign(item.mem_size(), 0)
> > - else:
> > - raise NotImplementedError("TODO minor check for
> > non-constant items")
> > -
> > - assert not want_extra_size
> > -
> > def write_validate_container(writer, prefix, container, start,
> > parent_scope, want_nw_size, want_mem_size, want_extra_size):
> > def prefix_m(prefix, m):
> > name = m.name
> > @@ -782,7 +760,7 @@ def write_parse_ptr_function(writer,
> > target_type):
> > writer.set_is_generated("parser", parse_function)
> >
> > writer = writer.function_helper()
> > - scope = writer.function(parse_function, "static uint8_t *",
> > "uint8_t *message_start, SPICE_GNUC_UNUSED uint8_t *message_end,
> > uint8_t *struct_data, PointerInfo *this_ptr_info, SPICE_GNUC_UNUSED
> > int minor")
> > + scope = writer.function(parse_function, "static uint8_t *",
> > "uint8_t *message_start, SPICE_GNUC_UNUSED uint8_t *message_end,
> > uint8_t *struct_data, PointerInfo *this_ptr_info")
> > scope.variable_def("uint8_t *", "in = message_start +
> > this_ptr_info->offset")
> > scope.variable_def("uint8_t *", "end")
> >
> > @@ -977,24 +955,7 @@ def write_member_parser(writer, container,
> > member, dest, scope):
> > def write_container_parser(writer, container, dest):
> > with dest.declare(writer) as scope:
> > for m in container.members:
> > - if m.has_minor_attr():
> > - writer.begin_block("if (minor >= %s)" %
> > m.get_minor_attr())
> > write_member_parser(writer, container, m, dest, scope)
> > - if m.has_minor_attr():
> > - # We need to zero out the fixed part of all optional
> > fields
> > - if not m.member_type.is_array():
> > - writer.end_block(newline=False)
> > - writer.begin_block(" else")
> > - # TODO: This is not right for fields that don't
> > exist in the struct
> > - if m.has_attr("zero"):
> > - pass
> > - elif m.member_type.is_primitive():
> > - writer.assign(dest.get_ref(m.name), "0")
> > - elif m.is_fixed_sizeof():
> > - writer.statement("memset ((char *)&%s, 0,
> > %s)" % (dest.get_ref(m.name), m.sizeof()))
> > - else:
> > - raise NotImplementedError("TODO Clear
> > optional dynamic fields")
> > - writer.end_block()
> >
> > def write_ptr_info_check(writer):
> > writer.newline()
> > @@ -1009,7 +970,7 @@ def write_ptr_info_check(writer):
> > writer.comment("Align to 32 bit").newline()
> > writer.assign("end", "(uint8_t
> > *)SPICE_ALIGN((size_t)end, 4)")
> > writer.assign("*%s" % dest, "(void *)end")
> > - writer.assign("end", "%s(message_start, message_end,
> > end, &ptr_info[%s], minor)" % (function, index))
> > + writer.assign("end", "%s(message_start, message_end,
> > end, &ptr_info[%s])" % (function, index))
> > writer.error_check("end == NULL")
> > writer.newline()
> >
> > @@ -1037,7 +998,7 @@ def write_msg_parser(writer, message):
> > writer.ifdef(message.attributes["ifdef"][0])
> > parent_scope = writer.function(function_name,
> > "uint8_t *",
> > - "uint8_t *message_start, uint8_t
> > *message_end, SPICE_GNUC_UNUSED int minor, size_t *size,
> > message_destructor_t *free_message", True)
> > + "uint8_t *message_start, uint8_t
> > *message_end, size_t *size, message_destructor_t *free_message",
> > True)
> > parent_scope.variable_def("SPICE_GNUC_UNUSED uint8_t *", "pos")
> > parent_scope.variable_def("uint8_t *", "start = message_start")
> > parent_scope.variable_def("uint8_t *", "data = NULL")
> > @@ -1160,7 +1121,7 @@ def write_channel_parser(writer, channel,
> > server):
> > for r in ranges:
> > d = d + 1
> > with writer.if_block("message_type >= %d && message_type <
> > %d" % (r[0], r[1]), d > 1, False):
> > - writer.statement("return funcs%d[message_type-
> > %d](message_start, message_end, minor, size_out, free_message)" % (d,
> > r[0]))
> > + writer.statement("return funcs%d[message_type-
> > %d](message_start, message_end, size_out, free_message)" % (d, r[0]))
> > writer.newline()
> >
> > writer.statement("return NULL")
> > diff --git a/python_modules/ptypes.py b/python_modules/ptypes.py
> > index f39f044..c548a28 100644
> > --- a/python_modules/ptypes.py
> > +++ b/python_modules/ptypes.py
> > @@ -15,46 +15,6 @@ def lookup_type(name):
> > def get_named_types():
> > return _types
> >
>
> This hunk seems to belong to the previous patch?
>
No, in this case FixedSize is an attribute of fields/structures/
messages, see below for usage.
> > -class FixedSize:
> > - def __init__(self, val = 0, minor = 0):
> > - if isinstance(val, FixedSize):
> > - self.vals = val.vals
> > - else:
> > - self.vals = [0] * (minor + 1)
> > - self.vals[minor] = val
> > -
> > - def __add__(self, other):
> > - if isinstance(other, int):
> > - other = FixedSize(other)
> > -
> > - new = FixedSize()
> > - l = max(len(self.vals), len(other.vals))
> > - shared = min(len(self.vals), len(other.vals))
> > -
> > - new.vals = [0] * l
> > -
> > - for i in range(shared):
> > - new.vals[i] = self.vals[i] + other.vals[i]
> > -
> > - for i in range(shared,len(self.vals)):
> > - new.vals[i] = self.vals[i]
> > -
> > - for i in range(shared,len(other.vals)):
> > - new.vals[i] = new.vals[i] + other.vals[i]
> > -
> > - return new
> > -
> > - def __radd__(self, other):
> > - return self.__add__(other)
> > -
> > - def __str__(self):
> > - s = "%d" % (self.vals[0])
> > -
> > - for i in range(1,len(self.vals)):
> > - if self.vals[i] > 0:
> > - s = s + " + ((minor >= %d)?%d:0)" % (i,
> > self.vals[i])
> > - return s
> > -
> > # Some attribute are propagated from member to the type as they
> > really
> > # are part of the type definition, rather than the member. This
> > applies
> > # only to attributes that affect pointer or array attributes, as
> > these
> > @@ -107,8 +67,6 @@ valid_attributes=set([
> > # when marshalling, a zero field is written to the network
> > # when demarshalling, the field is read from the network and
> > discarded
> > 'zero',
> > - # specify minor version required for these members
> > - 'minor',
> > # this attribute does not exist on the network, fill just
> > structure with the value
> > 'virtual',
> > ])
> > @@ -119,7 +77,6 @@ attributes_with_arguments=set([
> > 'as_ptr',
> > 'outvar',
> > 'ifdef',
> > - 'minor',
> > 'virtual',
> > ])
> >
> > @@ -587,15 +544,9 @@ class Containee:
> > raise Exception('attribute %s not expected' % name)
> > return name in self.attributes
> >
> > - def has_minor_attr(self):
> > - return self.has_attr("minor")
> > -
> > def has_end_attr(self):
> > return self.has_attr("end")
> >
> > - def get_minor_attr(self):
> > - return self.attributes["minor"][0]
> > -
> > class Member(Containee):
> > def __init__(self, name, member_type, attribute_list):
> > Containee.__init__(self)
> > @@ -635,9 +586,6 @@ class Member(Containee):
> > if self.has_attr("virtual"):
> > return 0
> > size = self.member_type.get_fixed_nw_size()
> > - if self.has_minor_attr():
> > - minor = self.get_minor_attr()
> > - size = FixedSize(size, minor)
Here is the only place that the FixedSize class is instantiated.
> > return size
> >
> > def contains_extra_size(self):
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel