* [Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message
@ 2019-11-27 8:53 Chris Sosnin
2019-11-27 22:55 ` Vladislav Shpilevoy
0 siblings, 1 reply; 6+ messages in thread
From: Chris Sosnin @ 2019-11-27 8:53 UTC (permalink / raw)
To: tarantool-patches
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
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
---
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/errcode.h b/src/box/errcode.h
index c660b1c70..97603259b 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -80,7 +80,7 @@ struct errcode_record {
/* 25 */_(ER_UPDATE_SPLICE, "SPLICE error on field %s: %s") \
/* 26 */_(ER_UPDATE_ARG_TYPE, "Argument type in operation '%c' on field %s does not match field type: expected %s") \
/* 27 */_(ER_FORMAT_MISMATCH_INDEX_PART, "Field %s has type '%s' in space format, but type '%s' in index definition") \
- /* 28 */_(ER_UNKNOWN_UPDATE_OP, "Unknown UPDATE operation") \
+ /* 28 */_(ER_UNKNOWN_UPDATE_OP, "Unknown UPDATE operation #%d: %s") \
/* 29 */_(ER_UPDATE_FIELD, "Field %s UPDATE error: %s") \
/* 30 */_(ER_FUNCTION_TX_ACTIVE, "Transaction is active at return from function") \
/* 31 */_(ER_KEY_PART_COUNT, "Invalid key part count (expected [0..%u], got %u)") \
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
@@ -172,8 +172,8 @@ xrow_update_read_ops(struct xrow_update *update, const char *expr,
}
struct xrow_update_op *op = update->ops;
struct xrow_update_op *ops_end = op + update->op_count;
- for (; op < ops_end; op++) {
- if (xrow_update_op_decode(op, update->index_base, dict,
+ for (int i = 1; op < ops_end; op++, i++) {
+ if (xrow_update_op_decode(op, i, update->index_base, dict,
&expr) != 0)
return -1;
/*
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;
+ }
+ 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;
+ 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);
+ 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,
struct tuple_dictionary *dict, const char **expr);
/* }}} xrow_update_op */
diff --git a/test/box/update.result b/test/box/update.result
index c6a5a25a7..317a51aca 100644
--- a/test/box/update.result
+++ b/test/box/update.result
@@ -826,11 +826,13 @@ s:update({0}, {{}})
...
s:update({0}, {{'+'}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got
+ 1'
...
s:update({0}, {{'+', 0}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got
+ 2'
...
s:update({0}, {{'+', '+', '+'}})
---
diff --git a/test/engine/upsert.result b/test/engine/upsert.result
index 47da307fa..5610c675d 100644
--- a/test/engine/upsert.result
+++ b/test/engine/upsert.result
@@ -1809,7 +1809,7 @@ space:upsert({1}, {{'!', 2, 100}}) -- must fail on checking tuple
...
space:upsert({'a'}, {{'a', 2, 100}}) -- must fail on checking ops
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: "a"'
...
space:upsert({'a'}, {{'!', 2, 'ups1'}}) -- 'fast' upsert via insert in one index
---
@@ -2117,19 +2117,22 @@ test(t, {{'+', 3, 3}, {'&', 3, 'a'}}, {{1, '1', 1, '1'}})
...
test(t, {{'+', 3}, {'&', 3, 'a'}}, {{1, '1', 1, '1'}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got
+ 2'
...
test(t, {{':', 3, 3}}, {{1, '1', 1, '1'}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 5, got
+ 3'
...
test(t, {{':', 3, 3, 3}}, {{1, '1', 1, '1'}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 5, got
+ 4'
...
test(t, {{'?', 3, 3}}, {{1, '1', 1, '1'}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: "?"'
...
'dump ' .. anything_to_string(box.space.s:select{})
---
diff --git a/test/vinyl/gh.result b/test/vinyl/gh.result
index 78cb2a28d..59578ebdd 100644
--- a/test/vinyl/gh.result
+++ b/test/vinyl/gh.result
@@ -369,7 +369,8 @@ s:select()
...
s:upsert({1, 'test', 'failed'}, {{'=', 3, 33}, {'=', 4, nil}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #2: wrong number of arguments, expected 3, got
+ 2'
...
s:select()
---
--
2.21.0 (Apple Git-122.2)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message
2019-11-27 8:53 [Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message Chris Sosnin
@ 2019-11-27 22:55 ` Vladislav Shpilevoy
2019-11-28 12:28 ` Chris Sosnin
0 siblings, 1 reply; 6+ messages in thread
From: Vladislav Shpilevoy @ 2019-11-27 22:55 UTC (permalink / raw)
To: Chris Sosnin, tarantool-patches
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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message
2019-11-27 22:55 ` Vladislav Shpilevoy
@ 2019-11-28 12:28 ` Chris Sosnin
2019-11-29 23:25 ` Vladislav Shpilevoy
0 siblings, 1 reply; 6+ messages in thread
From: Chris Sosnin @ 2019-11-28 12:28 UTC (permalink / raw)
To: v.shpilevoy, tarantool-patches
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:
+ 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;
Also I fixed coding style and extra white space.
And finally
> 5. You added an argument, but didn't update the comment.
+ * @param op_num Ordinal number of the opearion
Commit message and the patch itself are below.
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
---
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-3884-update-error
issue: https://github.com/tarantool/tarantool/issues/3884
src/box/errcode.h | 2 +-
src/box/xrow_update.c | 4 ++--
src/box/xrow_update_field.c | 25 +++++++++++++++++--------
src/box/xrow_update_field.h | 3 ++-
test/box/update.result | 6 ++++--
test/engine/upsert.result | 13 ++++++++-----
test/vinyl/gh.result | 3 ++-
7 files changed, 36 insertions(+), 20 deletions(-)
diff --git a/src/box/errcode.h b/src/box/errcode.h
index c660b1c70..97603259b 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -80,7 +80,7 @@ struct errcode_record {
/* 25 */_(ER_UPDATE_SPLICE, "SPLICE error on field %s: %s") \
/* 26 */_(ER_UPDATE_ARG_TYPE, "Argument type in operation '%c' on field %s does not match field type: expected %s") \
/* 27 */_(ER_FORMAT_MISMATCH_INDEX_PART, "Field %s has type '%s' in space format, but type '%s' in index definition") \
- /* 28 */_(ER_UNKNOWN_UPDATE_OP, "Unknown UPDATE operation") \
+ /* 28 */_(ER_UNKNOWN_UPDATE_OP, "Unknown UPDATE operation #%d: %s") \
/* 29 */_(ER_UPDATE_FIELD, "Field %s UPDATE error: %s") \
/* 30 */_(ER_FUNCTION_TX_ACTIVE, "Transaction is active at return from function") \
/* 31 */_(ER_KEY_PART_COUNT, "Invalid key part count (expected [0..%u], got %u)") \
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
@@ -172,8 +172,8 @@ xrow_update_read_ops(struct xrow_update *update, const char *expr,
}
struct xrow_update_op *op = update->ops;
struct xrow_update_op *ops_end = op + update->op_count;
- for (; op < ops_end; op++) {
- if (xrow_update_op_decode(op, update->index_base, dict,
+ for (int i = 1; op < ops_end; op++, i++) {
+ if (xrow_update_op_decode(op, i, update->index_base, dict,
&expr) != 0)
return -1;
/*
diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c
index c694e17e9..b5a68d24c 100644
--- a/src/box/xrow_update_field.c
+++ b/src/box/xrow_update_field.c
@@ -576,9 +576,15 @@ 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) {
+error:
+ diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num,
+ tt_sprintf("\"%.*s\"", len, opcode));
+ return NULL;
+ }
+ switch (*opcode) {
case '=':
return &op_set;
case '+':
@@ -595,13 +601,12 @@ xrow_update_op_by(char opcode)
case '!':
return &op_insert;
default:
- diag_set(ClientError, ER_UNKNOWN_UPDATE_OP);
- return NULL;
+ goto 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,
struct tuple_dictionary *dict, const char **expr)
{
if (mp_typeof(**expr) != MP_ARRAY) {
@@ -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);
+ 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..e87bd6fab 100644
--- a/src/box/xrow_update_field.h
+++ b/src/box/xrow_update_field.h
@@ -189,6 +189,7 @@ struct xrow_update_op {
/**
* Decode an update operation from MessagePack.
* @param[out] op Update operation.
+ * @param op_num Ordinal number of the opearion
* @param index_base Field numbers base: 0 or 1.
* @param dict Dictionary to lookup field number by a name.
* @param expr MessagePack.
@@ -197,7 +198,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,
struct tuple_dictionary *dict, const char **expr);
/* }}} xrow_update_op */
diff --git a/test/box/update.result b/test/box/update.result
index c6a5a25a7..317a51aca 100644
--- a/test/box/update.result
+++ b/test/box/update.result
@@ -826,11 +826,13 @@ s:update({0}, {{}})
...
s:update({0}, {{'+'}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got
+ 1'
...
s:update({0}, {{'+', 0}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got
+ 2'
...
s:update({0}, {{'+', '+', '+'}})
---
diff --git a/test/engine/upsert.result b/test/engine/upsert.result
index 47da307fa..5610c675d 100644
--- a/test/engine/upsert.result
+++ b/test/engine/upsert.result
@@ -1809,7 +1809,7 @@ space:upsert({1}, {{'!', 2, 100}}) -- must fail on checking tuple
...
space:upsert({'a'}, {{'a', 2, 100}}) -- must fail on checking ops
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: "a"'
...
space:upsert({'a'}, {{'!', 2, 'ups1'}}) -- 'fast' upsert via insert in one index
---
@@ -2117,19 +2117,22 @@ test(t, {{'+', 3, 3}, {'&', 3, 'a'}}, {{1, '1', 1, '1'}})
...
test(t, {{'+', 3}, {'&', 3, 'a'}}, {{1, '1', 1, '1'}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got
+ 2'
...
test(t, {{':', 3, 3}}, {{1, '1', 1, '1'}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 5, got
+ 3'
...
test(t, {{':', 3, 3, 3}}, {{1, '1', 1, '1'}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 5, got
+ 4'
...
test(t, {{'?', 3, 3}}, {{1, '1', 1, '1'}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #1: "?"'
...
'dump ' .. anything_to_string(box.space.s:select{})
---
diff --git a/test/vinyl/gh.result b/test/vinyl/gh.result
index 78cb2a28d..59578ebdd 100644
--- a/test/vinyl/gh.result
+++ b/test/vinyl/gh.result
@@ -369,7 +369,8 @@ s:select()
...
s:upsert({1, 'test', 'failed'}, {{'=', 3, 33}, {'=', 4, nil}})
---
-- error: Unknown UPDATE operation
+- error: 'Unknown UPDATE operation #2: wrong number of arguments, expected 3, got
+ 2'
...
s:select()
---
--
2.21.0 (Apple Git-122.2)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message
2019-11-28 12:28 ` Chris Sosnin
@ 2019-11-29 23:25 ` Vladislav Shpilevoy
2019-11-30 0:09 ` Chris Sosnin
0 siblings, 1 reply; 6+ messages in thread
From: Vladislav Shpilevoy @ 2019-11-29 23:25 UTC (permalink / raw)
To: Chris Sosnin, tarantool-patches
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message
2019-11-29 23:25 ` Vladislav Shpilevoy
@ 2019-11-30 0:09 ` Chris Sosnin
2019-11-30 1:04 ` Vladislav Shpilevoy
0 siblings, 1 reply; 6+ messages in thread
From: Chris Sosnin @ 2019-11-30 0:09 UTC (permalink / raw)
To: v.shpilevoy, tarantool-patches
Sorry for my mistake again, here is the diff:
+ const char *str = tt_sprintf("wrong number of arguments, "\
+ "expected %u, got %u",
+ op->meta->arg_count, arg_count);
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-3884-update-error
issue: https://github.com/tarantool/tarantool/issues/3884
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message
2019-11-30 0:09 ` Chris Sosnin
@ 2019-11-30 1:04 ` Vladislav Shpilevoy
0 siblings, 0 replies; 6+ messages in thread
From: Vladislav Shpilevoy @ 2019-11-30 1:04 UTC (permalink / raw)
To: Chris Sosnin, tarantool-patches, Kirill Yukhin
Thanks for the fixes!
LGTM.
On 30/11/2019 01:09, Chris Sosnin wrote:
> Sorry for my mistake again, here is the diff:
> + const char *str = tt_sprintf("wrong number of arguments, "\
> + "expected %u, got %u",
> + op->meta->arg_count, arg_count);
>
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-3884-update-error
> issue: https://github.com/tarantool/tarantool/issues/3884
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-30 1:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-27 8:53 [Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message Chris Sosnin
2019-11-27 22:55 ` Vladislav Shpilevoy
2019-11-28 12:28 ` Chris Sosnin
2019-11-29 23:25 ` Vladislav Shpilevoy
2019-11-30 0:09 ` Chris Sosnin
2019-11-30 1:04 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox