Signed-off-by: luzhixing12345 <[email protected]>
>On Sun, Aug 04, 2024 at 10:23:53PM GMT, luzhixing12345 wrote:
>>rewrite with if-else instead of goto
>
>Why?
>
>IMHO was better before this patch with a single error path.
I think this if-else version is more clear for me, and it's good to
keep things the way they are.
>
>>
>>and I have a question, in two incorrent cases
>>
>>- need reply but no reply_requested
>>- no need reply but has reply_requested
>>
>>should we call vu_panic or print warning message?
>>
>>---
>> subprojects/libvhost-user/libvhost-user.c | 39 +++++++++++++----------
>> subprojects/libvhost-user/libvhost-user.h | 6 ++--
>> 2 files changed, 27 insertions(+), 18 deletions(-)
>>
>>diff --git a/subprojects/libvhost-user/libvhost-user.c
>>b/subprojects/libvhost-user/libvhost-user.c
>>index 9c630c2170..187e25f9bb 100644
>>--- a/subprojects/libvhost-user/libvhost-user.c
>>+++ b/subprojects/libvhost-user/libvhost-user.c
>>@@ -2158,32 +2158,39 @@ vu_dispatch(VuDev *dev)
>> {
>> VhostUserMsg vmsg = { 0, };
>> int reply_requested;
>>- bool need_reply, success = false;
>>+ bool need_reply, success = true;
>>
>> if (!dev->read_msg(dev, dev->sock, &vmsg)) {
>>- goto end;
>>+ success = false;
>>+ free(vmsg.data);
>>+ return success;
>> }
>>
>> need_reply = vmsg.flags & VHOST_USER_NEED_REPLY_MASK;
>>
>> reply_requested = vu_process_message(dev, &vmsg);
>>- if (!reply_requested && need_reply) {
>>- vmsg_set_reply_u64(&vmsg, 0);
>>- reply_requested = 1;
>>- }
>>-
>>- if (!reply_requested) {
>>- success = true;
>>- goto end;
>>- }
>>
>>- if (!vu_send_reply(dev, dev->sock, &vmsg)) {
>>- goto end;
>>+ if (need_reply) {
>>+ if (reply_requested) {
>>+ if (!vu_send_reply(dev, dev->sock, &vmsg)) {
>>+ success = false;
>>+ }
>>+ } else {
>>+ // need reply but no reply requested, return 0(u64)
>>+ vmsg_set_reply_u64(&vmsg, 0);
>>+ if (!vu_send_reply(dev, dev->sock, &vmsg)) {
>>+ success = false;
>>+ }
>>+ }
>>+ } else {
>>+ // no need reply but reply requested, send a reply
>>+ if (reply_requested) {
>>+ if (!vu_send_reply(dev, dev->sock, &vmsg)) {
>>+ success = false;
>>+ }
>>+ }
>> }
>>
>>- success = true;
>>-
>>-end:
>> free(vmsg.data);
>> return success;
>> }
>>diff --git a/subprojects/libvhost-user/libvhost-user.h
>>b/subprojects/libvhost-user/libvhost-user.h
>>index deb40e77b3..2daf8578f6 100644
>>--- a/subprojects/libvhost-user/libvhost-user.h
>>+++ b/subprojects/libvhost-user/libvhost-user.h
>>@@ -238,6 +238,8 @@ typedef struct VuDev VuDev;
>>
>> typedef uint64_t (*vu_get_features_cb) (VuDev *dev);
>> typedef void (*vu_set_features_cb) (VuDev *dev, uint64_t features);
>>+typedef uint64_t (*vu_get_protocol_features_cb) (VuDev *dev);
>>+typedef void (*vu_set_protocol_features_cb) (VuDev *dev, uint64_t features);
>
>Are these changes related?
>
>Stefano
>
>> typedef int (*vu_process_msg_cb) (VuDev *dev, VhostUserMsg *vmsg,
>> int *do_reply);
>> typedef bool (*vu_read_msg_cb) (VuDev *dev, int sock, VhostUserMsg *vmsg);
>>@@ -256,9 +258,9 @@ typedef struct VuDevIface {
>> vu_set_features_cb set_features;
>> /* get the protocol feature bitmask from the underlying vhost
>> * implementation */
>>- vu_get_features_cb get_protocol_features;
>>+ vu_get_protocol_features_cb get_protocol_features;
>> /* enable protocol features in the underlying vhost implementation. */
>>- vu_set_features_cb set_protocol_features;
>>+ vu_set_protocol_features_cb set_protocol_features;
>> /* process_msg is called for each vhost-user message received */
>> /* skip libvhost-user processing if return value != 0 */
>> vu_process_msg_cb process_msg;
>>--
>>2.34.1
>>
Yes, I'm sorry that I forget to message about it.
Although get/set_protocol_features and get/set_protocol_features actually have
the same type, I think the return type of function pointers should be explicit
for user interface APIs. So typedef vu_get_protocol_features_cb and
vu_set_protocol_features_cb
luzhixing
