Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Modify mismatched error types when get_item_info failed #739

Merged
merged 1 commit into from
Mar 11, 2024
Merged

FIX: Modify mismatched error types when get_item_info failed #739

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
273 changes: 132 additions & 141 deletions memcached.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3212,16 +3212,13 @@ static void complete_update_ascii(conn *c)
}

item *it = c->item;
ENGINE_ERROR_CODE ret;
if (!mc_engine.v1->get_item_info(mc_engine.v0, c, it, &c->hinfo)) {
mc_engine.v1->release(mc_engine.v0, c, it);
ing-eoking marked this conversation as resolved.
Show resolved Hide resolved
mc_logger->log(EXTENSION_LOG_WARNING, c,
"%d: Failed to get item info\n", c->sfd);
out_string(c, "SERVER_ERROR failed to get item details");
return;
}

ENGINE_ERROR_CODE ret;
if (hinfo_check_ascii_tail_string(&c->hinfo) != 0) { /* check "\r\n" */
out_string(c, "SERVER_ERROR out of memory for getting item info");
ret = ENGINE_ENOMEM;
} else if (hinfo_check_ascii_tail_string(&c->hinfo) != 0) { /* check "\r\n" */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래 형태가 좋을 것 같은 데, 어떤가요?

  • 앞의 if 에서는 어떤 오류인지를 알고 있어 out_string 바로 호출하고,
    마지막 else 문에서만 switch 사용하여 오류에 따라 메세지를 선택한다.
  • ENOMEM도 그 의미가 다를 수 있어, 메세지를 다르게 할 수 있다.
    • get_item_info() 오류의 메세지
    • store()에서 slab memory 부족시의 메세지
    item *it = c->item;
    ENGINE_ERROR_CODE ret;
    if (!mc_engine.v1->get_item_info(mc_engine.v0, c, it, &c->hinfo)) {
        mc_logger->log(EXTENSION_LOG_WARNING, c,
                       "%d: Failed to get item info\n", c->sfd);
        out_string(c, "SERVER_ERROR out of memory for getting item info")
        ret = ENGINE_ENOMEM;
    } else if (hinfo_check_ascii_tail_string(&c->hinfo) != 0) { /* check "\r\n" */
        out_string(c, "CLIENT_ERROR bad data chunk");
        ret = ENGINE_EBADVALUE;
    } else {
        . . .
    }

    if (c->store_op == OPERATION_CAS) {
        update_stat_cas(c, ret);
    } else {
        STATS_CMD(c, set, c->hinfo.key, c->hinfo.nkey);
    }
    mc_engine.v1->release(mc_engine.v0, c, c->item);
    c->item = 0;

This comment was marked as resolved.

Sign in to view

out_string(c, "CLIENT_ERROR bad data chunk");
ret = ENGINE_EBADVALUE;
} else {
Expand Down Expand Up @@ -3705,87 +3702,86 @@ static void complete_update_bin(conn *c)
assert(c != NULL);

item *it = c->item;
ENGINE_ERROR_CODE ret;
if (!mc_engine.v1->get_item_info(mc_engine.v0, c, it, &c->hinfo)) {
mc_engine.v1->release(mc_engine.v0, c, it);
mc_logger->log(EXTENSION_LOG_WARNING, c,
"%d: Failed to get item info\n", c->sfd);
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EINTERNAL, 0);
return;
}
/* We don't actually receive the trailing two characters in the bin
* protocol, so we're going to just append them here */
hinfo_append_ascii_tail_string(&c->hinfo); /* append "\r\n" */
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, 0);

This comment was marked as outdated.

Sign in to view

This comment was marked as off-topic.

Sign in to view

ret = ENGINE_ENOMEM;
} else {
/* We don't actually receive the trailing two characters in the bin
* protocol, so we're going to just append them here */
hinfo_append_ascii_tail_string(&c->hinfo); /* append "\r\n" */

ENGINE_ERROR_CODE ret;
ret = mc_engine.v1->store(mc_engine.v0, c, it, &c->cas, c->store_op,
c->binary_header.request.vbucket);
CONN_CHECK_AND_SET_EWOULDBLOCK(ret, c);
ret = mc_engine.v1->store(mc_engine.v0, c, it, &c->cas, c->store_op,
c->binary_header.request.vbucket);
CONN_CHECK_AND_SET_EWOULDBLOCK(ret, c);
#ifdef ENABLE_DTRACE
switch (c->cmd) {
case OPERATION_ADD:
MEMCACHED_COMMAND_ADD(c->sfd, c->hinfo.key, c->hinfo.nkey,
(ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas);
break;
case OPERATION_REPLACE:
MEMCACHED_COMMAND_REPLACE(c->sfd, c->hinfo.key, c->hinfo.nkey,
switch (c->cmd) {
case OPERATION_ADD:
MEMCACHED_COMMAND_ADD(c->sfd, c->hinfo.key, c->hinfo.nkey,
(ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas);
break;
case OPERATION_APPEND:
MEMCACHED_COMMAND_APPEND(c->sfd, c->hinfo.key, c->hinfo.nkey,
(ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas);
break;
case OPERATION_PREPEND:
MEMCACHED_COMMAND_PREPEND(c->sfd, c->hinfo.key, c->hinfo.nkey,
break;
case OPERATION_REPLACE:
MEMCACHED_COMMAND_REPLACE(c->sfd, c->hinfo.key, c->hinfo.nkey,
(ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas);
break;
case OPERATION_APPEND:
MEMCACHED_COMMAND_APPEND(c->sfd, c->hinfo.key, c->hinfo.nkey,
(ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas);
break;
case OPERATION_PREPEND:
MEMCACHED_COMMAND_PREPEND(c->sfd, c->hinfo.key, c->hinfo.nkey,
(ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas);
break;
case OPERATION_SET:
MEMCACHED_COMMAND_SET(c->sfd, c->hinfo.key, c->hinfo.nkey,
(ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas);
break;
case OPERATION_SET:
MEMCACHED_COMMAND_SET(c->sfd, c->hinfo.key, c->hinfo.nkey,
(ret == ENGINE_SUCCESS) ? c->hinfo.nbytes : -1, c->cas);
break;
}
break;
}
#endif

switch (ret) {
case ENGINE_SUCCESS:
write_bin_response(c, NULL, 0, 0, 0);
break;
case ENGINE_KEY_EEXISTS:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_KEY_EEXISTS, 0);
break;
case ENGINE_KEY_ENOENT:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_KEY_ENOENT, 0);
break;
case ENGINE_NOT_STORED:
/* FIXME: check below code, later. */
if (c->store_op == OPERATION_ADD) {
switch (ret) {
case ENGINE_SUCCESS:
write_bin_response(c, NULL, 0, 0, 0);
break;
case ENGINE_KEY_EEXISTS:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_KEY_EEXISTS, 0);
} else if (c->store_op == OPERATION_REPLACE) {
break;
case ENGINE_KEY_ENOENT:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_KEY_ENOENT, 0);
} else {
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_NOT_STORED, 0);
break;
case ENGINE_NOT_STORED:
/* FIXME: check below code, later. */
if (c->store_op == OPERATION_ADD) {
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_KEY_EEXISTS, 0);
} else if (c->store_op == OPERATION_REPLACE) {
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_KEY_ENOENT, 0);
} else {
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_NOT_STORED, 0);
}
break;
case ENGINE_PREFIX_ENAME:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_PREFIX_ENAME, 0);
break;
case ENGINE_ENOMEM:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, 0);
break;
case ENGINE_EINVAL:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EINVAL, 0);
break;
case ENGINE_E2BIG:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_E2BIG, 0);
break;
case ENGINE_NOT_MY_VBUCKET:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_NOT_MY_VBUCKET, 0);
break;
case ENGINE_EBADTYPE:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EBADTYPE, 0);
break;
default:
handle_unexpected_errorcode_bin(c, __func__, ret, 0);
}
//write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_NOT_STORED, 0);
break;
case ENGINE_PREFIX_ENAME:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_PREFIX_ENAME, 0);
break;
case ENGINE_ENOMEM:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, 0);
break;
case ENGINE_EINVAL:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EINVAL, 0);
break;
case ENGINE_E2BIG:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_E2BIG, 0);
break;
case ENGINE_NOT_MY_VBUCKET:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_NOT_MY_VBUCKET, 0);
break;
case ENGINE_EBADTYPE:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EBADTYPE, 0);
break;
default:
handle_unexpected_errorcode_bin(c, __func__, ret, 0);
}

if (c->store_op == OPERATION_CAS) {
Expand Down Expand Up @@ -3829,7 +3825,7 @@ static void process_bin_get(conn *c)
mc_engine.v1->release(mc_engine.v0, c, it);
mc_logger->log(EXTENSION_LOG_WARNING, c,
"%d: Failed to get item info\n", c->sfd);
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EINTERNAL, 0);
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, 0);
break;
}

Expand Down Expand Up @@ -7104,41 +7100,43 @@ static void process_bin_update(conn *c)
req->message.body.flags,
realtime(req->message.body.expiration),
c->binary_header.request.cas);
if (ret == ENGINE_SUCCESS && !mc_engine.v1->get_item_info(mc_engine.v0,
c, it, &c->hinfo)) {
mc_engine.v1->release(mc_engine.v0, c, it);
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EINTERNAL, 0);
return;
}
if (ret == ENGINE_SUCCESS) {
if (!mc_engine.v1->get_item_info(mc_engine.v0, c, it, &c->hinfo)) {
mc_engine.v1->release(mc_engine.v0, c, it);
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, vlen);
ret = ENGINE_ENOMEM;
} else {
if (c->cmd == PROTOCOL_BINARY_CMD_ADD)
c->store_op = OPERATION_ADD;
else if (c->cmd == PROTOCOL_BINARY_CMD_SET)
c->store_op = OPERATION_SET;
else if (c->cmd == PROTOCOL_BINARY_CMD_REPLACE)
c->store_op = OPERATION_REPLACE;
else
assert(0);

switch (ret) {
case ENGINE_SUCCESS:
if (c->cmd == PROTOCOL_BINARY_CMD_ADD)
c->store_op = OPERATION_ADD;
else if (c->cmd == PROTOCOL_BINARY_CMD_SET)
c->store_op = OPERATION_SET;
else if (c->cmd == PROTOCOL_BINARY_CMD_REPLACE)
c->store_op = OPERATION_REPLACE;
else
assert(0);
if (c->binary_header.request.cas != 0) {
c->store_op = OPERATION_CAS;
}

if (c->binary_header.request.cas != 0) {
c->store_op = OPERATION_CAS;
c->item = it;
ritem_set_first(c, CONN_RTYPE_HINFO, vlen);
conn_set_state(c, conn_nread);
c->substate = bin_read_set_value;
}

c->item = it;
ritem_set_first(c, CONN_RTYPE_HINFO, vlen);
conn_set_state(c, conn_nread);
c->substate = bin_read_set_value;
break;
case ENGINE_E2BIG:
case ENGINE_ENOMEM:
if (ret == ENGINE_E2BIG) {
} else {
switch (ret) {
case ENGINE_E2BIG:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_E2BIG, vlen);
} else {
break;
case ENGINE_ENOMEM:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, vlen);
break;
default:
handle_unexpected_errorcode_bin(c, __func__, ret, vlen);
}

}
if (ret != ENGINE_SUCCESS) {
/*
* Avoid stale data persisting in cache because we failed alloc.
* Unacceptable for SET (but only if cas matches).
Expand All @@ -7158,9 +7156,6 @@ static void process_bin_update(conn *c)
c->noreply = false;
}
}
break;
default:
handle_unexpected_errorcode_bin(c, __func__, ret, vlen);
}
}

Expand All @@ -7186,37 +7181,34 @@ static void process_bin_append_prepend(conn *c)
ENGINE_ERROR_CODE ret;
ret = mc_engine.v1->allocate(mc_engine.v0, c, &it, key, nkey, vlen+2,
0, 0, c->binary_header.request.cas);
if (ret == ENGINE_SUCCESS && !mc_engine.v1->get_item_info(mc_engine.v0,
c, it, &c->hinfo)) {
mc_engine.v1->release(mc_engine.v0, c, it);
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_EINTERNAL, 0);
return;
}

switch (ret) {
case ENGINE_SUCCESS:
if (c->cmd == PROTOCOL_BINARY_CMD_APPEND)
c->store_op = OPERATION_APPEND;
else if (c->cmd == PROTOCOL_BINARY_CMD_PREPEND)
c->store_op = OPERATION_PREPEND;
else
assert(0);
if (ret == ENGINE_SUCCESS) {
if (!mc_engine.v1->get_item_info(mc_engine.v0, c, it, &c->hinfo)) {
mc_engine.v1->release(mc_engine.v0, c, it);
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, vlen);
} else {
if (c->cmd == PROTOCOL_BINARY_CMD_APPEND)
c->store_op = OPERATION_APPEND;
else if (c->cmd == PROTOCOL_BINARY_CMD_PREPEND)
c->store_op = OPERATION_PREPEND;
else
assert(0);

c->item = it;
ritem_set_first(c, CONN_RTYPE_HINFO, vlen);
conn_set_state(c, conn_nread);
c->substate = bin_read_set_value;
break;
case ENGINE_E2BIG:
case ENGINE_ENOMEM:
if (ret == ENGINE_E2BIG) {
c->item = it;
ritem_set_first(c, CONN_RTYPE_HINFO, vlen);
conn_set_state(c, conn_nread);
c->substate = bin_read_set_value;
}
} else {
switch (ret) {
case ENGINE_E2BIG:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_E2BIG, vlen);
} else {
break;
case ENGINE_ENOMEM:
write_bin_packet(c, PROTOCOL_BINARY_RESPONSE_ENOMEM, vlen);
break;
default:
handle_unexpected_errorcode_bin(c, __func__, ret, vlen);
}
break;
default:
handle_unexpected_errorcode_bin(c, __func__, ret, vlen);
}
}

Expand Down Expand Up @@ -8475,25 +8467,24 @@ static void process_update_command(conn *c, token_t *tokens, const size_t ntoken
if (!mc_engine.v1->get_item_info(mc_engine.v0, c, it, &c->hinfo)) {
mc_engine.v1->release(mc_engine.v0, c, it);
out_string(c, "SERVER_ERROR error getting item data");
ret = ENGINE_FAILED; /* FIXME: error type */
ret = ENGINE_ENOMEM;

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

} else {
c->item = it;
ritem_set_first(c, CONN_RTYPE_HINFO, vlen);
c->store_op = store_op;
conn_set_state(c, conn_nread);
}
}
if (ret != ENGINE_SUCCESS) {
} else {
if (ret == ENGINE_E2BIG) {
out_string(c, "CLIENT_ERROR object too large for cache");
} else if (ret == ENGINE_ENOMEM) {
out_string(c, "SERVER_ERROR out of memory storing object");
} else if (ret == ENGINE_FAILED) {
/* out_string() was called above. so, do nothing */
} else {
handle_unexpected_errorcode_ascii(c, __func__, ret);
}
}

if (ret != ENGINE_SUCCESS) {
/* Avoid stale data persisting in cache because we failed alloc.
* Unacceptable for SET. Anywhere else too?
*/
Expand Down
Loading