qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v14 04/21] socket: export socket_get_fd() function


From: Marc-André Lureau
Subject: Re: [PATCH v14 04/21] socket: export socket_get_fd() function
Date: Fri, 18 Dec 2020 15:39:44 +0400

Hi

On Fri, Dec 18, 2020 at 7:59 AM <elena.ufimtseva@oracle.com> wrote:
From: Jagannathan Raman <jag.raman@oracle.com>

Export socket_get_fd() helper function. The function protorype is
changed to be more generic. Use monitor_fd_param() instead of
monitor_fd_get() in order to support named fds as well.

Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
---
 include/qemu/sockets.h    |  1 +
 stubs/monitor.c           |  2 +-
 tests/test-util-sockets.c |  2 +-
 util/qemu-sockets.c       | 18 ++++++++++--------
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 7d1f813576..d87cf9f1c5 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -41,6 +41,7 @@ int unix_listen(const char *path, Error **errp);
 int unix_connect(const char *path, Error **errp);

 SocketAddress *socket_parse(const char *str, Error **errp);
+int socket_get_fd(const char *fdstr, Error **errp);
 int socket_connect(SocketAddress *addr, Error **errp);
 int socket_listen(SocketAddress *addr, int num, Error **errp);
 void socket_listen_cleanup(int fd, Error **errp);
diff --git a/stubs/monitor.c b/stubs/monitor.c
index 20786ac4ff..6e6850cd3a 100644
--- a/stubs/monitor.c
+++ b/stubs/monitor.c
@@ -3,7 +3,7 @@
 #include "monitor/monitor.h"
 #include "../monitor/monitor-internal.h"

-int monitor_get_fd(Monitor *mon, const char *name, Error **errp)
+int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp)
 {
     error_setg(errp, "only QEMU supports file descriptor passing");
     return -1;
diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c
index 67486055ed..e790a0af25 100644
--- a/tests/test-util-sockets.c
+++ b/tests/test-util-sockets.c
@@ -54,7 +54,7 @@ static int mon_fd = -1;
 static const char *mon_fdname;
 __thread Monitor *cur_mon;

-int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
+int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp)
 {
     g_assert(cur_mon);
     g_assert(mon == cur_mon);
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 8af0278f15..694552cb7d 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1116,16 +1116,12 @@ fail:
     return NULL;
 }

-static int socket_get_fd(const char *fdstr, int num, Error **errp)
+int socket_get_fd(const char *fdstr, Error **errp)
 {
     Monitor *cur_mon = monitor_cur();
     int fd;
-    if (num != 1) {
-        error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections");
-        return -1;
-    }
     if (cur_mon) {
-        fd = monitor_get_fd(cur_mon, fdstr, errp);
+        fd = monitor_fd_param(cur_mon, fdstr, errp);

It's not a good idea to change 2 things in the same patch. Although here the change is fairly small, it may have consequences that are unrelated, and we may want to revert just that.

Here, if the fd name is numeric, it will break current users. So I am afraid we can't change it like that. Perhaps add a flag like "named_fd_only" and call one or the other?

On a related note, I find it quite confusing that we have -add-fd using fdset, but qmp 'getfd' for named fd (and no command line version?). Some commands can use fdset (all with file path, since /dev/fdset/ makes it accessible), while others rely on monitor_get_fd(). It's an area that could benefit cleanups and better API/docs. That's not for you to do though, just me mumbling.

 
         if (fd < 0) {
             return -1;
         }
@@ -1159,7 +1155,7 @@ int socket_connect(SocketAddress *addr, Error **errp)
         break;

     case SOCKET_ADDRESS_TYPE_FD:
-        fd = socket_get_fd(addr->u.fd.str, 1, errp);
+        fd = socket_get_fd(addr->u.fd.str, errp);
         break;

     case SOCKET_ADDRESS_TYPE_VSOCK:
@@ -1177,6 +1173,12 @@ int socket_listen(SocketAddress *addr, int num, Error **errp)
     int fd;

     trace_socket_listen(num);
+
+    if (num != 1) {
+        error_setg_errno(errp, EINVAL, "socket_get_fd: too many connections");

You should also change the error message to locate socket_listen instead.

+        return -1;
+    }
+
     switch (addr->type) {
     case SOCKET_ADDRESS_TYPE_INET:
         fd = inet_listen_saddr(&addr->u.inet, 0, num, errp);
@@ -1187,7 +1189,7 @@ int socket_listen(SocketAddress *addr, int num, Error **errp)
         break;

     case SOCKET_ADDRESS_TYPE_FD:
-        fd = socket_get_fd(addr->u.fd.str, num, errp);
+        fd = socket_get_fd(addr->u.fd.str, errp);
         break;

     case SOCKET_ADDRESS_TYPE_VSOCK:
--
2.25.GIT



--
Marc-André Lureau

reply via email to

[Prev in Thread] Current Thread [Next in Thread]