On 1/4/2024 9:04 AM, Michal Prívozník wrote:
On 11/30/23 00:11, Praveen K Paladugu wrote:
virSocketSendMsgWithFDs method send fds along with payload using
SCM_RIGHTS. virSocketRecvHttpResponse method polls, receives http response
on the input socket and returns the http response code.
These methods are required to add network suppport in ch driver.
Signed-off-by: Praveen K Paladugu <prapal@xxxxxxxxxxxxxxxxxxx>
---
po/POTFILES | 1 +
src/libvirt_private.syms | 2 +
src/util/virsocket.c | 109 +++++++++++++++++++++++++++++++++++++++
src/util/virsocket.h | 3 ++
4 files changed, 115 insertions(+)
diff --git a/po/POTFILES b/po/POTFILES
index 023c041f61..b594a8dd39 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -326,6 +326,7 @@ src/util/virscsi.c
src/util/virscsihost.c
src/util/virscsivhost.c
src/util/virsecret.c
+src/util/virsocket.c
src/util/virsocketaddr.c
src/util/virstoragefile.c
src/util/virstring.c
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 680a90034a..e643cea774 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3371,7 +3371,9 @@ virSecureEraseString;
# util/virsocket.h
virSocketRecvFD;
+virSocketRecvHttpResponse;
virSocketSendFD;
+virSocketSendMsgWithFDs;
# util/virsocketaddr.h
diff --git a/src/util/virsocket.c b/src/util/virsocket.c
index cd6f7ecd1b..b11e53a215 100644
--- a/src/util/virsocket.c
+++ b/src/util/virsocket.c
@@ -22,8 +22,14 @@
#include "virsocket.h"
#include "virutil.h"
#include "virfile.h"
+#include "virlog.h"
#include <fcntl.h>
+#include <poll.h>
+
+#define PKT_TIMEOUT_MS 500 /* ms */
+
+VIR_LOG_INIT("util.virsocket");
#ifdef WIN32
@@ -482,6 +488,109 @@ virSocketRecvFD(int sock, int fdflags)
return fd;
}
+
+/**
+ * virSocketSendMsgWithFDs:
+ * @sock: socket to send payload and fds to
+ * @payload: payload to send
+ * @fds: array of fds to send
+ * @fds_len: len of fds array
+
+ * Send @fds along with @payload to @sock using SCM_RIGHTS.
+ * Return number of bytes sent on success, or -1 on error.
+ */
+int
+virSocketSendMsgWithFDs(int sock, const char *payload, int *fds, size_t fds_len)
+{
+ struct msghdr msg;
+ struct iovec iov[1]; /* Send a single payload, so set vector len to 1 */
+ int ret;
+ char control[CMSG_SPACE(sizeof(int)*fds_len)];
+ struct cmsghdr *cmsg;
+
+ memset(&msg, 0, sizeof(msg));
+ memset(control, 0, sizeof(control));
+
+ iov[0].iov_base = (void *) payload;
+ iov[0].iov_len = strlen(payload);
+
+ msg.msg_iov = iov;
+ msg.msg_iovlen = 1;
+
+ msg.msg_control = control;
+ msg.msg_controllen = sizeof(control);
+
+ cmsg = CMSG_FIRSTHDR(&msg);
+ if (!cmsg) {
+ VIR_ERROR(_("Couldn't fit control msg header in msg"));
I don't think this can ever be true. But if you want to keep this check,
I suggest you ditch the VIR_ERROR() call and replace it with setting errno.
I did this following the below excerpt in cmsg(3) manpage:
CMSG_FIRSTHDR()
returns a pointer to the first cmsghdr in the ancillary
data buffer associated with the passed msghdr. It returns
NULL if there isn't enough space for a cmsghdr in the
buffer.
As `msg` is being allocated with enough memory, I see this will not
happen. I will drop this error msg.
We don't really use VIR_ERROR() after logging subsystem was initialized.
And a function can either report no errors (and leave it up to caller)
or report errors in all cases. Simirarly, because of sendmsg() retval
check below, caller should use virReportSystemError() (because errno is
set on sendmsg() failure), so other error paths should set some sensible
errno too.
I will drop using VIR_ERROR and set errno instead.
+ return -1;
+ }
+ cmsg->cmsg_len = CMSG_LEN(sizeof(int) * fds_len);
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
I believe SCM_RIGHT is undefined on anything else than Linux. Just like
virSocketSendFD() we need alternative, dummy impl. Otherwise, WIN32
compilation fails since this function is defined only in !WIN32 block.
I did not checked WIN32 builds. I will fix this.
+ memcpy(CMSG_DATA(cmsg), fds, sizeof(int) * fds_len);
+
+ do {
+ ret = sendmsg(sock, &msg, 0);
+ } while (ret < 0 && errno == EINTR);
+
+ if (ret < 0)
+ return -1;
+
+ return ret;
+}
+
+/**
+ * virSocketRecvHttpResponse:
+ * @sock: socket to receive http response on
+ *
+ * This function polls @sock for HTTP response
+ * Returns HTTP response code from received message, or -1 on error.
+ */
+int virSocketRecvHttpResponse(int sock)
This is something that should live in CH driver code.
+{
+ struct pollfd pfds[1];
+ /* This is only used for responses from ch guests, which fit within
+ * 1024 buffer
+ */
+ char buf[1024];
+ int response_code, ret;
+
+ pfds[0].fd = sock;
+ pfds[0].events = POLLIN;
+
+ do {
+ ret = poll(pfds, G_N_ELEMENTS(pfds), PKT_TIMEOUT_MS);
+ } while (ret < 0 && errno == EINTR);
+
+ if (ret <= 0) {
+ if (ret < 0) {
+ VIR_ERROR(_("Poll on sock %1$d failed"), sock);
+ } else if (ret == 0) {
+ VIR_ERROR(_("Poll on sock %1$d timed out"), sock);
+ }
+ return -1;
+ }
+
+ do {
+ ret = recv(sock, buf, sizeof(buf), 0);
So if we receive more than 1024 bytes, then the 'buf' is not '\0'
terminated ... [1]
+ } while (ret < 0 && errno == EINTR);
+
+ if (ret < 0) {
+ VIR_ERROR(_("recv on sock %1$d failed"), sock);
+ return -1;
+ }
+
Until here it's generic enough so that it could live under src/util/,
but ... [2]
+ /* Parse the HTTP response code */
+ ret = sscanf(buf, "HTTP/1.%*d %d", &response_code);
1: ... and this will cause us to read beyond array boundaries. What we
usually do in this case is:
ret = recv(sock, buf, sizeof(buf) - 1, 0);
buf[ret] = '\0';
Alternatively, you may init the buffer with zeroes:
char buf[1024] = { 0 };
ret = recv(sock, buf, sizeof(buf) - 1, 0);
I will adopt this option, seems clean and easy to follow.
+ if (ret != 1) {
+ VIR_ERROR(_("Failed to parse HTTP response code"));
+ return -1;
+ }
2: ... this is too specific IMO.
As I put this method in src/util, I tried to keep it as generic as
possible. Wait for a response, read it and parse response code.
I could refactor it to accept a `buf`, `size` arguments and return
buffer to ch to parse the response code. Does that sound resonable?
Also, for esx driver, which also uses HTTP to talk to the hypervisor, we
use libcurl. But I'm not sure if it can do FD passing. I doubt it can.
It's okay to construct HTTP requests ourselves (just like you're doing
in 5/5) then. Just a thought.
libcurl cannot do FD passing. That is why I had to implement these util
methods.
+
+ return response_code;
+}
+
Michal
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
--
Regards,
Praveen
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
