[Spice-devel] [PATCH spice-common] build: Remove FIXME_SERVER_SMARTCARD hack

[Eduardo Lima (Etrunko)]

On 10/05/18 12:01, Frediano Ziglio wrote:
>>
>> Ping.
>>
>> The meson port depends on this patch. I tried the testing procedure
>> described in the link below, but could not make it work. Has anyone ever
>> made it work? I could use some help here.
>>
>> Regards, Eduardo.
>>
>> https://www.spice-space.org/smartcard-usage.html
>>
>> On 02/03/18 12: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.
>>>
>>> Signed-off-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
>>> ---
>>>  common/Makefile.am          | 2 --
>>>  configure.ac                | 7 -------
>>>  python_modules/demarshal.py | 2 +-
>>>  spice.proto                 | 2 +-
>>>  4 files changed, 2 insertions(+), 11 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 3542161..5230223 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -63,11 +63,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..9377869 100644
>>> --- a/python_modules/demarshal.py
>>> +++ b/python_modules/demarshal.py
>>> @@ -1042,7 +1042,7 @@ def write_msg_parser(writer, message):
>>>      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())
>>> +                         or not (message.members[0].is_array() and
>>> message.has_attr("nocopy")))
>>>  
>>>      writer.newline()
>>>      if message.has_attr("ifdef"):
>>> diff --git a/spice.proto b/spice.proto
>>> index 76cfef2..6226c67 100644
>>> --- a/spice.proto
>>> +++ b/spice.proto
>>> @@ -1416,7 +1416,7 @@ 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);
>>>  
>>>
>>
> 
> 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))

Better to stick with a different define then, instead of using the same
result if dependency is found. I will remove the AH_BOTTOM, and change
the define, while keeping the syntax fixes, in the case someone in the
future really want to build that piece of 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.
> 
> Frediano
> 

Thanks for the review.

-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com