From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 181DA46970F for ; Thu, 28 Nov 2019 01:55:18 +0300 (MSK) References: <20191127085325.66144-1-k.sosnin@tarantool.org> From: Vladislav Shpilevoy Message-ID: <6550bdaf-9233-49e8-4919-8b334b5c81e1@tarantool.org> Date: Wed, 27 Nov 2019 23:55:15 +0100 MIME-Version: 1.0 In-Reply-To: <20191127085325.66144-1-k.sosnin@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chris Sosnin , tarantool-patches@dev.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); >