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.
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@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
