[PATCH] vhost-user: rewrite vu_dispatch with if-else

[PATCH] vhost-user: rewrite vu_dispatch with if-else

[luzhixing12345]

rewrite with if-else instead of goto

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);
 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

Re: [PATCH] vhost-user: rewrite vu_dispatch with if-else

[Stefano Garzarella]

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.

>
>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
>

Re: [PATCH] vhost-user: rewrite vu_dispatch with if-else

[luzhixing12345]

Signed-off-by: luzhixing12345 <luzhixing12345@gmail.com>

>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

Re: [PATCH] vhost-user: rewrite vu_dispatch with if-else

[Michael S. Tsirkin]

On Mon, Aug 05, 2024 at 11:27:27PM +0800, luzhixing12345 wrote:
> Signed-off-by: luzhixing12345 <luzhixing12345@gmail.com>
> 
> >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.

Whoever writes code always thinks his version is the clearest.
Code should be written with reader in mind.
Stefano happens to be the reviewer, so pls make things clear for him,
not for you.

> >
> >>
> >>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

Re: [PATCH] vhost-user: rewrite vu_dispatch with if-else

[Michael S. Tsirkin]

On Sun, Aug 04, 2024 at 10:23:53PM +0800, luzhixing12345 wrote:
> rewrite with if-else instead of goto
> 
> 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?

this is not how you post a patch to the list.


> ---
>  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);
>  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