Re: [Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch, this is better now!

On 27/11/2019 09:53, Chris Sosnin wrote:
> Thank you for the review!
> See second version below.
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-3884-update-error
> issue: https://github.com/tarantool/tarantool/issues/3884

Please, read the coding style tutorial more carefully.

- When you send a new version of a patchset, you should write
  a list of changes. So as it would be simpler for a reviewer
  to remember what was wrong;

- When you send a one-commit patchset, all your comments,
  links, whatever else, goes below '---'. Otherwise a reviewer
  can't understand where a commit message starts. And 'git am'
  does not work with that.

> 
> Calling tuple_object:update() with invalid argument number
> yields 'Unknown UPDATE operation' error. Instead, we replace this
> error with explicit "wrong argument number", mentioning which operation
> failed. Also add checking for correct opcode.
> 
> Fixes #3884
> ---

You should have written the links here.

See 5 comments below.

>  src/box/errcode.h           |  2 +-
>  src/box/xrow_update.c       |  4 ++--
>  src/box/xrow_update_field.c | 23 ++++++++++++++++-------
>  src/box/xrow_update_field.h |  2 +-
>  test/box/update.result      |  6 ++++--
>  test/engine/upsert.result   | 13 ++++++++-----
>  test/vinyl/gh.result        |  3 ++-
>  7 files changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c
> index c694e17e9..47c263945 100644
> --- a/src/box/xrow_update_field.c
> +++ b/src/box/xrow_update_field.c
> @@ -576,9 +576,14 @@ static const struct xrow_update_op_meta op_delete = {
>  };
>  
>  static inline const struct xrow_update_op_meta *
> -xrow_update_op_by(char opcode)
> +xrow_update_op_by(const char *opcode, uint32_t len, int op_num)
>  {
> -	switch (opcode) {
> +	if (len != 1) {
> +		diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num,
> +			 tt_sprintf("\"%.*s\"", len, opcode));
> +		return NULL;

1. To not duplicate code, you can add a label, and make a
goto to there from 'default'. Or vice versa - go to default
from there.

> +	}
> +	switch (*opcode) {
>  	case '=':
>  		return &op_set;
>  	case '+':
> @@ -595,13 +600,14 @@ xrow_update_op_by(char opcode)
>  	case '!':
>  		return &op_insert;
>  	default:
> -		diag_set(ClientError, ER_UNKNOWN_UPDATE_OP);
> +		diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num,
> +			 tt_sprintf("\"%.*s\"", len, opcode));
>  		return NULL;
>  	}
>  }
>  
>  int
> -xrow_update_op_decode(struct xrow_update_op *op, int index_base,
> +xrow_update_op_decode(struct xrow_update_op *op, int op_num, int index_base,
>  		      struct tuple_dictionary *dict, const char **expr)
>  {
>  	if (mp_typeof(**expr) != MP_ARRAY) {
> @@ -620,12 +626,15 @@ xrow_update_op_decode(struct xrow_update_op *op, int index_base,
>  			 "update operation name must be a string");
>  		return -1;
>  	}
> -	op->opcode = *mp_decode_str(expr, &len);
> -	op->meta = xrow_update_op_by(op->opcode);
> +	const char *opcode = mp_decode_str(expr, &len);
> +	op->opcode = *opcode;

2. I would better assign it after xrow_update_op_by()
returned not NULL. Because MessagePack strings are not
zero terminated. So 'opcode' after mp_decode_str() may
actually point at invalid memory instead of zero
terminator in case of an empty string.

> +	op->meta = xrow_update_op_by(opcode, len, op_num);
>  	if (op->meta == NULL)
>  		return -1;
>  	if (arg_count != op->meta->arg_count) {
> -		diag_set(ClientError, ER_UNKNOWN_UPDATE_OP);
> +		const char *str = tt_sprintf("wrong number of arguments, "\
> +			 "expected %u, got %u", op->meta->arg_count, arg_count);

3. Please, check coding guidelines and other similar places.
We always align multiline strings by '('. Here the word 'expected'
should be aligned right below 'wrong'. Like this:

const char *str = tt_sprintf("wrong number of arguments, "\
                             "expected %u, got %u",
                             op->meta->arg_count, arg_count);

Or like this:

const char *str =
        tt_sprintf("wrong number of arguments, expected %u, got %u",
                   op->meta->arg_count, arg_count);

> +		diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num, str);
>  		return -1;
>  	}
>  	int32_t field_no = 0;
> diff --git a/src/box/xrow_update_field.h b/src/box/xrow_update_field.h
> index e90095b9e..23a4ae517 100644
> --- a/src/box/xrow_update_field.h
> +++ b/src/box/xrow_update_field.h
> @@ -197,7 +197,7 @@ struct xrow_update_op {
>   * @retval -1 Client error.
>   */
>  int
> -xrow_update_op_decode(struct xrow_update_op *op, int index_base,
> +xrow_update_op_decode(struct xrow_update_op *op, int op_num,  int index_base,

4. Double whitespace after 'op_num,'.

5. You added an argument, but didn't update the comment.

>  		      struct tuple_dictionary *dict, const char **expr);
>