From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 119F046970F for ; Sat, 30 Nov 2019 02:25:10 +0300 (MSK) References: <6550bdaf-9233-49e8-4919-8b334b5c81e1@tarantool.org> <20191128122815.15968-1-k.sosnin@tarantool.org> From: Vladislav Shpilevoy Message-ID: <874875d2-9976-5e97-4898-9f2b6ddfe3fd@tarantool.org> Date: Sat, 30 Nov 2019 00:25:09 +0100 MIME-Version: 1.0 In-Reply-To: <20191128122815.15968-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 fixes! On 28/11/2019 13:28, Chris Sosnin wrote: > Hi! Thank you for your suggestions. > I am sorry for being impatient. > >> 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. > > I labeled the first return as error: > > +error: > + diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num, > + tt_sprintf("\"%.*s\"", len, opcode)); > + return NULL; > + } > ... > default: > - diag_set(ClientError, ER_UNKNOWN_UPDATE_OP); > - return NULL; > + goto error; > } > >> 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. > > In this case we would still return with an error, however > I changed it to be your way: It will return an error, yes. In case the dereference of the invalid pointer won't crash by luck. > > + const char *opcode = mp_decode_str(expr, &len); > + op->meta = xrow_update_op_by(opcode, len, op_num); > if (op->meta == NULL) > return -1; > + op->opcode = *opcode; > > diff --git a/src/box/xrow_update.c b/src/box/xrow_update.c > index 123db081a..db215aada 100644 > --- a/src/box/xrow_update.c > +++ b/src/box/xrow_update.c > @@ -620,12 +625,16 @@ 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->meta = xrow_update_op_by(opcode, len, op_num); > if (op->meta == NULL) > return -1; > + op->opcode = *opcode; > 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); Seems like the comment 3 from the previous review is not fixed. The indentation is still incorrect here. > + diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num, str); > return -1; > } > int32_t field_no = 0;