From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Tue, 4 Jun 2019 17:11:53 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH v3] Add MsgPack ext types handling.
Message-ID: <20190604141153.63z7yefi2uituyw3@esperanza>
References: <20190603131425.29405-1-sergepetrenko@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190603131425.29405-1-sergepetrenko@tarantool.org>
To: Serge Petrenko <sergepetrenko@tarantool.org>
Cc: tarantool-patches@freelists.org, georgy@tarantool.org, kostja@tarantool.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Mon, Jun 03, 2019 at 04:14:25PM +0300, Serge Petrenko wrote:
> diff --git a/msgpuck.c b/msgpuck.c
> index d0ffb83..e5f7e25 100644
> --- a/msgpuck.c
> +++ b/msgpuck.c
> @@ -33,6 +33,17 @@
>  #define MP_LIBRARY 1
>  #include "msgpuck.h"
>  
> +const char *
> +mp_ext_type_str(enum mp_ext_type type)
> +{
> +	switch(type) {
> +	case MP_EXT_TIMESTAMP:
> +		return "timestamp";
> +	default:
> +		return "undefined";
> +	}
> +}
> +
>  size_t
>  mp_vformat(char *data, size_t data_size, const char *format, va_list vl)
>  {
> @@ -298,9 +309,14 @@ next:										\
>  		PRINTF("%lg", mp_decode_double(&data));				\
>  		break;								\
>  	case MP_EXT:								\
> -		mp_next(&data);							\
> -		PRINTF("undefined");						\
> +	{									\
> +		int8_t type;							\
> +		uint32_t len;							\
> +		len = mp_decode_extl(&data, &type);				\
> +		PRINTF("%s", mp_ext_type_str(type));	    			\
> +		data += len;							\
>  		break;								\
> +	}									\

It isn't exactly helpful to see "timestamp" instead of a timestamp
value. If we add MP_EXT_TIMESTAMP to the library, we should implement
all methods needed to work with it:

  mp_sizeof_timestamp
  mp_encode_timestamp
  mp_decode_timestamp
  mp_vformat
  mp_snprint

Since this would require a lot of effort and is out of the scope of this
issue, I suggest dropping MP_EXT_TIMESTAMP altogether for now. Let's
just simply add mp_ext infrastructure, like you did in this patch. In
tests, you can pass an arbitrary number for MP_EXT type.

> +/**
> + * \brief Encode extension data of length \a len.
> + * The function is equivalent to mp_encode_extl() + memcpy.
> + * \param data - a buffer
> + * \param ext_data - a pointer to extension data
> + * \param type - extension type to encode
> + * \param len - a extension data length
> + * \return \a data + mp_sizeof_ext(\a len) ==
> + * data + mp_sizeof_extl(\a len) + \a len
> + * \sa mp_encode_strl
> + */
> +MP_PROTO char *
> +mp_encode_ext(char *data, char *ext_data, int8_t type, uint32_t len);

Nit:

 - IMO 'type' should go before 'ext_data' in the argument list as it
   defines what 'ext_data' is about.

 - I'd rename 'ext_data' argument to 'str' for consistency with
   mp_encode_bin/str. Besides, it's shorter. Not really important
   so this one is up to you.

Other than that, the patch looks good to me.