[Spice-devel] [PATCH spice-common v3] build: Remove FIXME_SERVER_SMARTCARD hack
Frediano Ziglio
fziglio at redhat.com
Fri May 11 16:35:36 UTC 2018
>
> On 11/05/18 10:14, Eduardo Lima (Etrunko) wrote:
> > When we remove the hacks in configure.ac and common/Makefile.am, two
> > errors pop out:
> >
> > generated_server_demarshallers.c: In function
> > ‘parse_msgc_smartcard_reader_add’:
> > generated_server_demarshallers.c:1985:30: error: ‘mem_size’ undeclared
> > (first use in this function); did you mean ‘nw_size’?
> > data = (uint8_t *)malloc(mem_size);
> > ^~~~~~~~
> > nw_size
> >
> > First one is caused by a missing declaration of mem_size, so we use the
> > same condition that causes this code to be added to the check for the
> > need of mem_size variable declaration in demarshal.py.
> >
> > generated_server_demarshallers.c:1985:30: note: each undeclared identifier
> > is reported only once for each function it appears in
> > generated_server_demarshallers.c:1994:15: error: ‘VSCMsgReaderAdd {aka
> > struct VSCMsgReaderAdd}’ has no member named ‘reader_name’
> > memcpy(out->reader_name, in, reader_name__nelements);
> > ^~
> >
> > This second one is only a rename of 'reader_name' field to 'name', as
> > specified in the VSCMsgReaderAdd structure in file vscard_common.h.
> >
> > N.B.: Even with those errors fixed, it looks like the generated code is
> > really broken and should not be built at all. So, following suggestion
> > after review in the mailing list, this patch also changes the define in
> > the code to be different from the one that is used to tell whether the
> > smartcard dependencies are found.
> >
> > https://lists.freedesktop.org/archives/spice-devel/2018-May/043395.html
> >
> > On 10/05/18 12:01, Frediano Ziglio wrote:
> >> I would just use the same ugly workaround.
> >> Do not define USE_SMARTCARD!
> >> (or change @ifdef(USE_SMARTCARD) to
> >> @ifdef(I_REALLY_WANT_TO_USE_THIS_BROKEN_CODE))
> >>
> >> The spice.proto for smart card is broken!
> >> First there are 4 message with same code (ok, this should be checked
> >> but is clearly an error).
> >> Second the messages are not defined in the same way they are encoded
> >> (manually!) by the current code (client and server).
> >> Apparently somebody try to use the protocol file and ended up
> >> commenting out the part relative to smart card.
> >> I got a working version but I don't like either, seems the combination
> >> required is not really handled by the Python code.
> >
> > Signed-off-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
> > ---
> > common/Makefile.am | 2 --
> > configure.ac | 7 -------
> > python_modules/demarshal.py | 3 +--
> > spice.proto | 15 +++++++++++++--
> > 4 files changed, 14 insertions(+), 13 deletions(-)
> >
> > diff --git a/common/Makefile.am b/common/Makefile.am
> > index 22aec80..ea15039 100644
> > --- a/common/Makefile.am
> > +++ b/common/Makefile.am
> > @@ -78,8 +78,6 @@ libspice_common_server_la_SOURCES = \
> > $(SERVER_MARSHALLERS) \
> > $(NULL)
> >
> > -libspice_common_server_la_CFLAGS = -DFIXME_SERVER_SMARTCARD
> > -
> > AM_CPPFLAGS = \
> > -I$(top_srcdir) \
> > -I$(top_builddir) \
> > diff --git a/configure.ac b/configure.ac
> > index 3da85de..39faf1c 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -64,11 +64,4 @@ AC_CONFIG_FILES([
> > docs/Makefile
> > ])
> >
> > -AH_BOTTOM([
> > -/* argh.. this is evil */
> > -#if defined(FIXME_SERVER_SMARTCARD) && defined(USE_SMARTCARD)
> > -%:undef USE_SMARTCARD
> > -#endif
> > -])
> > -
> > AC_OUTPUT
> > diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> > index da87d44..dc7a815 100644
> > --- a/python_modules/demarshal.py
> > +++ b/python_modules/demarshal.py
> > @@ -1041,8 +1041,7 @@ def write_msg_parser(writer, message):
> > msg_type = message.c_type()
> > msg_sizeof = message.sizeof()
> >
> > - want_mem_size = (len(message.members) != 1 or
> > message.members[0].is_fixed_nw_size()
> > - or not message.members[0].is_array())
> > + want_mem_size = not message.has_attr("nocopy")
> >
> > writer.newline()
> > if message.has_attr("ifdef"):
> > diff --git a/spice.proto b/spice.proto
> > index 6f873a2..baa2eec 100644
> > --- a/spice.proto
> > +++ b/spice.proto
> > @@ -1423,9 +1423,20 @@ channel SmartcardChannel : BaseChannel {
> > } @ctype(VSCMsgATR) atr = 101;
> >
> > message {
> > - int8 reader_name[] @zero_terminated @nonnull;
> > + int8 name[] @zero_terminated @nonnull;
> > } @ctype(VSCMsgReaderAdd) reader_add = 101;
> > -} @ifdef(USE_SMARTCARD);
> > +
> > +/* The protocol specification for SmartcardChannel as it is, does not
> > work:
> > + * - All messages have the same ID, which causes some message definitions
> > + * to be ignored;
> > + * - The structure of the resulting message is not the same used by the
> > code,
> > + * so it would require a change in the protocol.
> > + *
> > + * For the above exposed, we still protect this code with #ifdef guards,
> > but
> > + * using a different define than the one used to check if the smartcard
> > + * dependencies are found.
> > + */
> > +} @ifdef(FIXME_SERVER_SMARTCARD); //@ifdef(USE_SMARTCARD);
>
>
> I have just noticed that this causes a build failure in spice-gtk, so it
> is better to keep the define as is at the moment. At least until we find
> somewhat final solution to this issue.
>
> Regards, Eduardo.
>
This should work (tested without Meson):
diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
index 8d3f5cb..7b53361 100644
--- a/python_modules/demarshal.py
+++ b/python_modules/demarshal.py
@@ -1039,8 +1039,7 @@ def write_msg_parser(writer, message):
msg_type = message.c_type()
msg_sizeof = message.sizeof()
- want_mem_size = (len(message.members) != 1 or message.members[0].is_fixed_nw_size()
- or not message.members[0].is_array())
+ want_mem_size = not message.has_attr("nocopy")
writer.newline()
if message.has_attr("ifdef"):
diff --git a/spice.proto b/spice.proto
index 4d916bb..58cdc49 100644
--- a/spice.proto
+++ b/spice.proto
@@ -1383,11 +1383,11 @@ struct VscMessageAPDU {
} @ctype(VSCMsgAPDU);
struct VscMessageATR {
- uint8 data[];
+ uint8 atr[];
} @ctype(VSCMsgATR);
struct VscMessageReaderAdd {
- int8 *reader_name[] @zero_terminated @nonnull @end @nomarshal;
+ int8 name[] @zero_terminated @nonnull @end @nomarshal;
} @ctype(VSCMsgReaderAdd);
channel SmartcardChannel : BaseChannel {
@@ -1400,6 +1400,7 @@ channel SmartcardChannel : BaseChannel {
} @ctype(SpiceMsgSmartcard) data = 101;
client:
+/*
message {
VscMessageHeader header;
switch (header.type) {
@@ -1412,13 +1413,13 @@ channel SmartcardChannel : BaseChannel {
VscMessageError error;
} u @anon;
} @ctype(SpiceMsgcSmartcard) data = 101;
-
+*/
message {
vsc_message_type type;
uint32 reader_id;
uint32 length;
} @ctype(VSCMsgHeader) header = 101;
-
+/*
message {
uint32 code;
} @ctype(VSCMsgError) error = 101;
@@ -1428,8 +1429,9 @@ channel SmartcardChannel : BaseChannel {
} @ctype(VSCMsgATR) atr = 101;
message {
- int8 reader_name[] @zero_terminated @nonnull;
+ int8 name[] @zero_terminated @nonnull;
} @ctype(VSCMsgReaderAdd) reader_add = 101;
+*/
} @ifdef(USE_SMARTCARD);
channel SpicevmcChannel : BaseChannel {
Needs some more comments.
Updates to structures should not be necessary but they
don't hurt.
Frediano
More information about the Spice-devel
mailing list