On Tue, 2018-10-09 at 17:14 -0400, Frediano Ziglio wrote:
> > 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."
"Note that exported functions retain the minor argument for
compatibility reasons".
But which exported functions are you referring to here?
>
> 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.
weird, ok.
>
> > > -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