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

[Eduardo Lima (Etrunko)]

On 11/05/18 08:54, Frediano Ziglio 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
>>
> 
> Maybe better to copy part of that message?
> 

Sure, I will add to the commit message for next version.


>> Signed-off-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
>> ---
>>  common/Makefile.am          | 2 --
>>  configure.ac                | 7 -------
>>  python_modules/demarshal.py | 2 +-
>>  spice.proto                 | 4 ++--
>>  4 files changed, 3 insertions(+), 12 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)		\
> 
> fine with this
> 
>> 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
> 
> great!
> 
>> 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"):
> 
> Looking at write_msg_parser and write_validate_container why this is
> not just
> 
> want_mem_size = not message.has_attr("nocopy")
> 
> ? I tried and just fix parse_msgc_smartcard_reader_add, as expected.
> 

Confirmed it works as well. Thanks for the pointer.

>> diff --git a/spice.proto b/spice.proto
>> index 6f873a2..632f798 100644
>> --- a/spice.proto
>> +++ b/spice.proto
>> @@ -1423,9 +1423,9 @@ channel SmartcardChannel : BaseChannel {
>>      } @ctype(VSCMsgATR) atr = 101;
>>  
>>      message {
>> -	int8 reader_name[] @zero_terminated @nonnull;
>> +	int8 name[] @zero_terminated @nonnull;
> 
> fine
> 
>>      } @ctype(VSCMsgReaderAdd) reader_add = 101;
>> -} @ifdef(USE_SMARTCARD);
>> +} @ifdef(FIXME_SERVER_SMARTCARD) //@ifdef(USE_SMARTCARD);
>>  
> 
> Fine (beside the missing ";") but I would add some comments here (before the @ifdef like maybe?)
> 
> /* The protocol specification for SmartcardChannel is broken and not
>  * working:
>  * - different messages have the same ID causing some message definition
>  *   to be ignored;
>  * - the structure of the resulting message is not the same used by
>  *   the code so would be a change in the protocol.
>  * This specification never worked.
>  */
> 

Done.


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