Re: [libvirt PATCH 3/5] util: Add util methods required by ch networking

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(a)linux.microsoft.com&gt; > --- > 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.

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.

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

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

> + if (ret != 1) { > + VIR_ERROR(_("Failed to parse HTTP response code")); > + return -1; > + } 2: ... this is too specific IMO.

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.

> + > + return response_code; > +} > + Michal _______________________________________________ Devel mailing list -- devel(a)lists.libvirt.org To unsubscribe send an email to devel-leave(a)lists.libvirt.org