qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] osdep: Introduce qemu_get_fd() to wrap the common codes

From: Guoyi Tu
Subject: Re: [PATCH] osdep: Introduce qemu_get_fd() to wrap the common codes
Date: Wed, 31 Aug 2022 16:47:37 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 

On 8/30/22 14:03, Markus Armbruster wrote:

Christian Schoenebeck <qemu_oss@crudebyte.com> writes:


On Donnerstag, 18. August 2022 14:06:04 CEST Guoyi Tu wrote:

Ping...

Any comments are welcome

On 8/12/22 19:01, Guoyi Tu wrote:

socket_get_fd() have much the same codes as monitor_fd_param(),
so qemu_get_fd() is introduced to implement the common logic.
now socket_get_fd() and monitor_fd_param() directly call this
function.


s/have/has/, s/now/Now/, some proper rephrasing wouldn't hurt either.


Signed-off-by: Guoyi Tu <tugy@chinatelecom.cn>
---

   include/qemu/osdep.h |  1 +
   monitor/misc.c       | 21 +--------------------
   util/osdep.c         | 25 +++++++++++++++++++++++++
   util/qemu-sockets.c  | 17 +++++------------
   4 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index b1c161c035..b920f128a7 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -491,6 +491,7 @@ int qemu_open_old(const char *name, int flags, ...);

   int qemu_open(const char *name, int flags, Error **errp);
   int qemu_create(const char *name, int flags, mode_t mode, Error **errp);
   int qemu_close(int fd);

+int qemu_get_fd(Monitor *mon, const char *fdname, Error **errp);

   int qemu_unlink(const char *name);
   #ifndef _WIN32
   int qemu_dup_flags(int fd, int flags);

diff --git a/monitor/misc.c b/monitor/misc.c
index 3d2312ba8d..0d3372cf2b 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -1395,26 +1395,7 @@ void monitor_fdset_dup_fd_remove(int dup_fd)

   int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp)
   {

-    int fd;
-    Error *local_err = NULL;
-
-    if (!qemu_isdigit(fdname[0]) && mon) {
-        fd = monitor_get_fd(mon, fdname, &local_err);
-    } else {
-        fd = qemu_parse_fd(fdname);
-        if (fd == -1) {
-            error_setg(&local_err, "Invalid file descriptor number '%s'",
-                       fdname);
-        }
-    }
-    if (local_err) {
-        error_propagate(errp, local_err);
-        assert(fd == -1);
-    } else {
-        assert(fd != -1);
-    }
-
-    return fd;
+    return qemu_get_fd(mon, fdname, errp);

   }


This becomes a trivial wrapper around qemu_get_fd().  Why do we need
both functions?
As Donnerstag said, the qemu_get_fd() in osdep.c is project wide function that is also needed by the test codes, such as test-unit-sockets.c and test-char.c. if direclty call monitor_fd_param() in socket_get_fd(), a stub version of monitor_fd_param() need to be defined for the test codes and adjust the overwritten api in test codes.
/* Please update hmp-commands.hx when adding or changing commands */ 

diff --git a/util/osdep.c b/util/osdep.c
index 60fcbbaebe..c57551ca78 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -23,6 +23,7 @@

    */
   #include "qemu/osdep.h"
   #include "qapi/error.h"

+#include "qemu/ctype.h"

   #include "qemu/cutils.h"
   #include "qemu/sockets.h"
   #include "qemu/error-report.h"

@@ -413,6 +414,30 @@ int qemu_close(int fd)

       return close(fd);
   }

+int qemu_get_fd(Monitor *mon, const char *fdname, Error **errp)
+{
+    int fd;
+    Error *local_err = NULL;
+
+    if (!qemu_isdigit(fdname[0]) && mon) {
+        fd = monitor_get_fd(mon, fdname, &local_err);
+    } else {
+        fd = qemu_parse_fd(fdname);
+        if (fd == -1) {
+            error_setg(&local_err, "Invalid file descriptor number '%s'",
+                       fdname);
+        }
+    }
+    if (local_err) {
+        error_propagate(errp, local_err);
+        assert(fd == -1);
+    } else {
+        assert(fd != -1);
+    }
+
+    return fd;
+}
+


Up to here you are basically just moving the code of monitor_fd_param() to a
project wide shared new function qemu_get_fd(), but why? I mean you could
simply call monitor_fd_param() in socket_get_fd() below, no?


Point.


   /*
    * Delete a file from the filesystem, unless the filename is

/dev/fdset/...

    *

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 13b5b197f9..92960ee6eb 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1142,19 +1142,12 @@ static int socket_get_fd(const char *fdstr,
Error **errp)

   {
       Monitor *cur_mon = monitor_cur();
       int fd;

-    if (cur_mon) {
-        fd = monitor_get_fd(cur_mon, fdstr, errp);
-        if (fd < 0) {
-            return -1;
-        }
-    } else {
-        if (qemu_strtoi(fdstr, NULL, 10, &fd) < 0) {
-            error_setg_errno(errp, errno,
-                             "Unable to parse FD number %s",
-                             fdstr);
-            return -1;
-        }
+
+    fd = qemu_get_fd(cur_mon, fdstr, errp);
+    if (fd < 0) {
+        return -1;

       }


This part looks like behaviour change to me. Haven't looked into the details
though whether it would be OK. Just saying.


When factoring out code that isn't obviously the same, it often makes
sense to first make it more obviously the same in-place, and only then
factor it out.

In this case: have PATCH 1/2 change socket_get_fd() in-place to make the
code obviously common, then de-duplicate it in PATCH 2/2.



Thanks for you advice,
If the codes is okay, i will send a patch set as suggested

Guoyi.






+


Unintentional white line added?



       if (!fd_is_socket(fd)) {
           error_setg(errp, "File descriptor '%s' is not a socket", fdstr);
           close(fd);
reply via email to
[Prev in Thread] Current Thread [Next in Thread]