On 12.03.2019 17:54, Maxime Coquelin wrote:
> External backends may have specific requests to handle, and so
> we don't want the vhost-user lib to handle these requests as
> errors.
>
> This patch also changes the experimental API by introducing
> RTE_VHOST_MSG_RESULT_NOT_HANDLED so that vhost-user lib
> can report an error if a message is handled neither by
> the vhost-user library nor by the external backend.
>
> The logic changes a bit so that if the callback returns
> with ERR, OK or REPLY, it is considered the message
> is handled by the external backend so it won't be
> handled by the vhost-user library.
> It is still possible for an external backend to listen
> to requests that have to be handled by the vhost-user
> library like SET_MEM_TABLE, but the callback have to
> return NOT_HANDLED in that case.
>
> Vhost-crypto backend is ialso adapted to this API change.
>
> Suggested-by: Ilya Maximets <i.maxim...@samsung.com>
> Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
> Tested-by: Darek Stojaczyk <dariusz.stojac...@intel.com>
> ---
> lib/librte_vhost/rte_vhost.h | 16 +++++--
> lib/librte_vhost/vhost_crypto.c | 10 +++-
> lib/librte_vhost/vhost_user.c | 82 +++++++++++++++++++++------------
> 3 files changed, 71 insertions(+), 37 deletions(-)
>
> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> index c9c392975..b1c5a0908 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -121,6 +121,8 @@ enum rte_vhost_msg_result {
> RTE_VHOST_MSG_RESULT_OK = 0,
> /* Message handling successful and reply prepared */
> RTE_VHOST_MSG_RESULT_REPLY = 1,
> + /* Message not handled */
> + RTE_VHOST_MSG_RESULT_NOT_HANDLED,
> };
>
> /**
> @@ -135,11 +137,13 @@ enum rte_vhost_msg_result {
> * If the handler requires skipping the master message handling, this
> variable
> * shall be written 1, otherwise 0.
Above statement should be updated because 'skip_master' removed.
BTW, maybe it's better to squash these two typedef's as they are
equal now? Comment parts that differs could be moved to the definition
of the 'struct rte_vhost_user_extern_ops'.
> * @return
> - * VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
> - * VH_RESULT_ERR on failure
> + * RTE_VHOST_MSG_RESULT_OK on success,
> + * RTE_VHOST_MSG_RESULT_REPLY on success with reply,
> + * RTE_VHOST_MSG_RESULT_ERR on failure,
> + * RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled.
> */
> typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid,
> - void *msg, uint32_t *skip_master);
> + void *msg);
>
> /**
> * Function prototype for the vhost backend to handler specific vhost user
> @@ -150,8 +154,10 @@ typedef enum rte_vhost_msg_result
> (*rte_vhost_msg_pre_handle)(int vid,
> * @param msg
> * Message pointer.
> * @return
> - * VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
> - * VH_RESULT_ERR on failure
> + * RTE_VHOST_MSG_RESULT_OK on success,
> + * RTE_VHOST_MSG_RESULT_REPLY on success with reply,
> + * RTE_VHOST_MSG_RESULT_ERR on failure,
> + * RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled.
> */
> typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid,
> void *msg);
> diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c
> index 0f437c4a1..9b4b850e8 100644
> --- a/lib/librte_vhost/vhost_crypto.c
> +++ b/lib/librte_vhost/vhost_crypto.c
> @@ -453,14 +453,20 @@ vhost_crypto_msg_post_handler(int vid, void *msg)
> return RTE_VHOST_MSG_RESULT_ERR;
> }
>
> - if (vmsg->request.master == VHOST_USER_CRYPTO_CREATE_SESS) {
> + switch (vmsg->request.master) {
> + case VHOST_USER_CRYPTO_CREATE_SESS:
> vhost_crypto_create_sess(vcrypto,
> &vmsg->payload.crypto_session);
> vmsg->fd_num = 0;
> ret = RTE_VHOST_MSG_RESULT_REPLY;
> - } else if (vmsg->request.master == VHOST_USER_CRYPTO_CLOSE_SESS) {
> + break;
> + case VHOST_USER_CRYPTO_CLOSE_SESS:
> if (vhost_crypto_close_sess(vcrypto, vmsg->payload.u64))
> ret = RTE_VHOST_MSG_RESULT_ERR;
> + break;
> + default:
> + ret = RTE_VHOST_MSG_RESULT_NOT_HANDLED;
> + break;
> }
>
> return ret;
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 555d09ad9..39756fce7 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1910,7 +1910,7 @@ vhost_user_msg_handler(int vid, int fd)
> int did = -1;
> int ret;
> int unlock_required = 0;
> - uint32_t skip_master = 0;
> + bool handled;
> int request;
>
> dev = get_device(vid);
> @@ -1928,27 +1928,30 @@ vhost_user_msg_handler(int vid, int fd)
> }
>
> ret = read_vhost_message(fd, &msg);
> - if (ret <= 0 || msg.request.master >= VHOST_USER_MAX) {
> + if (ret <= 0) {
> if (ret < 0)
> RTE_LOG(ERR, VHOST_CONFIG,
> "vhost read message failed\n");
> - else if (ret == 0)
> + else
> RTE_LOG(INFO, VHOST_CONFIG,
> "vhost peer closed\n");
> - else
> - RTE_LOG(ERR, VHOST_CONFIG,
> - "vhost read incorrect message\n");
>
> return -1;
> }
>
> ret = 0;
> - if (msg.request.master != VHOST_USER_IOTLB_MSG)
> - RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
> - vhost_message_str[msg.request.master]);
> - else
> - RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
> - vhost_message_str[msg.request.master]);
> + request = msg.request.master;
> + if (request > VHOST_USER_NONE && request < VHOST_USER_MAX &&
> + vhost_message_str[request]) {
> + if (request != VHOST_USER_IOTLB_MSG)
> + RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
> + vhost_message_str[request]);
> + else
> + RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
> + vhost_message_str[request]);
> + } else {
> + RTE_LOG(DEBUG, VHOST_CONFIG, "External request %d\n", request);
> + }
>
> ret = vhost_user_check_and_alloc_queue_pair(dev, &msg);
> if (ret < 0) {
> @@ -1964,7 +1967,7 @@ vhost_user_msg_handler(int vid, int fd)
> * inactive, so it is safe. Otherwise taking the access_lock
> * would cause a dead lock.
> */
> - switch (msg.request.master) {
> + switch (request) {
> case VHOST_USER_SET_FEATURES:
> case VHOST_USER_SET_PROTOCOL_FEATURES:
> case VHOST_USER_SET_OWNER:
> @@ -1989,19 +1992,24 @@ vhost_user_msg_handler(int vid, int fd)
>
> }
>
> + handled = false;
> if (dev->extern_ops.pre_msg_handle) {
> ret = (*dev->extern_ops.pre_msg_handle)(dev->vid,
> - (void *)&msg, &skip_master);
> - if (ret == RTE_VHOST_MSG_RESULT_ERR)
> - goto skip_to_reply;
> - else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
> + (void *)&msg);
> + switch (ret) {
> + case RTE_VHOST_MSG_RESULT_REPLY:
> send_vhost_reply(fd, &msg);
> -
> - if (skip_master)
> + /* Fall-through */
> + case RTE_VHOST_MSG_RESULT_ERR:
> + case RTE_VHOST_MSG_RESULT_OK:
> + handled = true;
> goto skip_to_post_handle;
> + case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
> + default:
> + break;
> + }
> }
>
> - request = msg.request.master;
> if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
> if (!vhost_message_handlers[request])
> goto skip_to_post_handle;
> @@ -2012,40 +2020,54 @@ vhost_user_msg_handler(int vid, int fd)
> RTE_LOG(ERR, VHOST_CONFIG,
> "Processing %s failed.\n",
> vhost_message_str[request]);
> + handled = true;
> break;
> case RTE_VHOST_MSG_RESULT_OK:
> RTE_LOG(DEBUG, VHOST_CONFIG,
> "Processing %s succeeded.\n",
> vhost_message_str[request]);
> + handled = true;
> break;
> case RTE_VHOST_MSG_RESULT_REPLY:
> RTE_LOG(DEBUG, VHOST_CONFIG,
> "Processing %s succeeded and needs reply.\n",
> vhost_message_str[request]);
> send_vhost_reply(fd, &msg);
> + handled = true;
> + break;
> + default:
> break;
> }
> - } else {
> - RTE_LOG(ERR, VHOST_CONFIG,
> - "Requested invalid message type %d.\n", request);
> - ret = RTE_VHOST_MSG_RESULT_ERR;
> }
>
> skip_to_post_handle:
> if (ret != RTE_VHOST_MSG_RESULT_ERR &&
> dev->extern_ops.post_msg_handle) {
> - ret = (*dev->extern_ops.post_msg_handle)(
> - dev->vid, (void *)&msg);
> - if (ret == RTE_VHOST_MSG_RESULT_ERR)
> - goto skip_to_reply;
> - else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
> + ret = (*dev->extern_ops.post_msg_handle)(dev->vid,
> + (void *)&msg);
> + switch (ret) {
> + case RTE_VHOST_MSG_RESULT_REPLY:
> send_vhost_reply(fd, &msg);
> + /* Fall-through */
> + case RTE_VHOST_MSG_RESULT_ERR:
> + case RTE_VHOST_MSG_RESULT_OK:
> + handled = true;
> + case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
> + default:
> + break;
> + }
> }
>
> -skip_to_reply:
> if (unlock_required)
> vhost_user_unlock_all_queue_pairs(dev);
>
> + /* If message was not handled at this stage, treat it as an error */
> + if (!handled) {
> + RTE_LOG(ERR, VHOST_CONFIG,
> + "vhost message (req: %d) was not handled.\n", request);
> + ret = RTE_VHOST_MSG_RESULT_ERR;
> + }
> +
> /*
> * If the request required a reply that was already sent,
> * this optional reply-ack won't be sent as the
>
