Re: [libvirt] [PATCH 3/5] Use helper functions to format the journal iov array

This simplifies the top-level code, at the cost of using a little more stack space. The primary benefit is being able to send more fields without knowing in advance how many of them, and of which types, these fields will be, and without having to individually add buffer variables. The code imposes an upper limit on the total number of iovs/buffers used, and fields that wouldn't fit are silently dropped. This is not significant in this patch, but will affect the following one. Signed-off-by: Miloslav Trmač <mitr(a)redhat.com&gt; --- src/util/logging.c | 144 ++++++++++++++++++++++++++++++++--------------------- 1 file changed, 87 insertions(+), 57 deletions(-) diff --git a/src/util/logging.c b/src/util/logging.c index a41ae8b..a6d00c9 100644 --- a/src/util/logging.c +++ b/src/util/logging.c @@ -1043,19 +1043,78 @@ virLogAddOutputToSyslog(virLogPriority priority, # if USE_JOURNALD -# define IOVEC_SET_STRING(iov, str) \ - do { \ - struct iovec *_i = &(iov); \ - _i->iov_base = (char*)str; \ - _i->iov_len = strlen(str); \ +# define IOVEC_SET(iov, data, size) \ + do { \ + struct iovec *_i = &(iov); \ + _i->iov_base = (void*)(data); \ + _i->iov_len = (size); \ } while (0) -# define IOVEC_SET_INT(iov, buf, val) \ - do { \ - struct iovec *_i = &(iov); \ - _i->iov_base = virFormatIntDecimal(buf, sizeof(buf), val); \ - _i->iov_len = strlen(buf); \ - } while (0) +# define IOVEC_SET_STRING(iov, str) IOVEC_SET(iov, str, strlen(str)) + +/* Used for conversion of numbers to strings, and for length of binary data */ +#define JOURNAL_BUF_SIZE (MAX(INT_BUFSIZE_BOUND(int), sizeof(uint64_t))) + +struct journalState +{ + struct iovec *iov, *iov_end; + char (*bufs)[JOURNAL_BUF_SIZE], (*bufs_end)[JOURNAL_BUF_SIZE]; +}; + +static void +journalAddString(struct journalState *state, const char *field, + const char *value) +{ + static const char newline = '\n', equals = '='; + + if (strchr(value, '\n') != NULL) { + uint64_t nstr; + + /* If 'str' contains a newline, then we must + * encode the string length, since we can't + * rely on the newline for the field separator + */ + if (state->iov_end - state->iov < 5 || state->bufs == state->bufs_end) + return; /* Silently drop */ + nstr = htole64(strlen(value)); + memcpy(state->bufs[0], &nstr, sizeof(nstr)); + + IOVEC_SET_STRING(state->iov[0], field); + IOVEC_SET(state->iov[1], &newline, sizeof(newline)); + IOVEC_SET(state->iov[2], state->bufs[0], sizeof(nstr)); + state->bufs++; + state->iov += 3; + } else { + if (state->iov_end - state->iov < 4) + return; /* Silently drop */ + IOVEC_SET_STRING(state->iov[0], field); + IOVEC_SET(state->iov[1], (void *)&equals, sizeof(equals)); + state->iov += 2; + } + IOVEC_SET_STRING(state->iov[0], value); + IOVEC_SET(state->iov[1], (void *)&newline, sizeof(newline)); + state->iov += 2; +} + +static void +journalAddInt(struct journalState *state, const char *field, int value) +{ + static const char newline = '\n', equals = '='; + + char *num; + + if (state->iov_end - state->iov < 4 || state->bufs == state->bufs_end) + return; /* Silently drop */ + + num = virFormatIntDecimal(state->bufs[0], sizeof(state->bufs[0]), value); + + IOVEC_SET_STRING(state->iov[0], field); + IOVEC_SET(state->iov[1], (void *)&equals, sizeof(equals)); + IOVEC_SET_STRING(state->iov[2], num); + IOVEC_SET(state->iov[3], (void *)&newline, sizeof(newline)); + state->bufs++; + state->iov += 4; +} static int journalfd = -1; @@ -1074,7 +1133,6 @@ virLogOutputToJournald(virLogSource source, { virCheckFlags(VIR_LOG_STACK_TRACE,); int buffd = -1; - size_t niov = 0; struct msghdr mh; struct sockaddr_un sa; union { @@ -1086,52 +1144,24 @@ virLogOutputToJournald(virLogSource source, * be a tmpfs, and one that is available from early boot on * and where unprivileged users can create files. */ char path[] = "/dev/shm/journal.XXXXXX"; - char priostr[INT_BUFSIZE_BOUND(priority)]; - char linestr[INT_BUFSIZE_BOUND(linenr)]; - - /* First message takes up to 4 iovecs, and each - * other field needs 3, assuming they don't have - * newlines in them - */ -# define IOV_SIZE (4 + (5 * 3)) - struct iovec iov[IOV_SIZE]; - - if (strchr(rawstr, '\n')) { - uint64_t nstr; - /* If 'str' contains a newline, then we must - * encode the string length, since we can't - * rely on the newline for the field separator - */ - IOVEC_SET_STRING(iov[niov++], "MESSAGE\n"); - nstr = htole64(strlen(rawstr)); - iov[niov].iov_base = (char*)&nstr; - iov[niov].iov_len = sizeof(nstr); - niov++; - } else { - IOVEC_SET_STRING(iov[niov++], "MESSAGE="); - } - IOVEC_SET_STRING(iov[niov++], rawstr); - IOVEC_SET_STRING(iov[niov++], "\n"); - - IOVEC_SET_STRING(iov[niov++], "PRIORITY="); - IOVEC_SET_INT(iov[niov++], priostr, priority); - IOVEC_SET_STRING(iov[niov++], "\n"); - - IOVEC_SET_STRING(iov[niov++], "LIBVIRT_SOURCE="); - IOVEC_SET_STRING(iov[niov++], virLogSourceTypeToString(source)); - IOVEC_SET_STRING(iov[niov++], "\n"); - IOVEC_SET_STRING(iov[niov++], "CODE_FILE="); - IOVEC_SET_STRING(iov[niov++], filename); - IOVEC_SET_STRING(iov[niov++], "\n"); +# define NUM_FIELDS 6 + struct iovec iov[NUM_FIELDS * 5]; + char iov_bufs[NUM_FIELDS][JOURNAL_BUF_SIZE]; + struct journalState state; - IOVEC_SET_STRING(iov[niov++], "CODE_LINE="); - IOVEC_SET_INT(iov[niov++], linestr, linenr); - IOVEC_SET_STRING(iov[niov++], "\n"); + state.iov = iov; + state.iov_end = iov + ARRAY_CARDINALITY(iov); + state.bufs = iov_bufs; + state.bufs_end = iov_bufs + ARRAY_CARDINALITY(iov_bufs); - IOVEC_SET_STRING(iov[niov++], "CODE_FUNC="); - IOVEC_SET_STRING(iov[niov++], funcname); - IOVEC_SET_STRING(iov[niov++], "\n"); + journalAddString(&state ,"MESSAGE", rawstr); + journalAddInt(&state, "PRIORITY", priority); + journalAddString(&state, "LIBVIRT_SOURCE", + virLogSourceTypeToString(source)); + journalAddString(&state, "CODE_FILE", filename); + journalAddInt(&state, "CODE_LINE", linenr); + journalAddString(&state, "CODE_FUNC", funcname); memset(&sa, 0, sizeof(sa)); sa.sun_family = AF_UNIX; @@ -1142,7 +1172,7 @@ virLogOutputToJournald(virLogSource source, mh.msg_name = &sa; mh.msg_namelen = offsetof(struct sockaddr_un, sun_path) + strlen(sa.sun_path); mh.msg_iov = iov; - mh.msg_iovlen = niov; + mh.msg_iovlen = state.iov - iov; if (sendmsg(journalfd, &mh, MSG_NOSIGNAL) >= 0) return; @@ -1166,7 +1196,7 @@ virLogOutputToJournald(virLogSource source, if (unlink(path) < 0) goto cleanup; - if (writev(buffd, iov, niov) < 0) + if (writev(buffd, iov, state.iov - iov) < 0) goto cleanup; mh.msg_iov = NULL;