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

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



On Wed, Oct 17, 2012 at 08:17:16PM +0200, Miloslav Trmač wrote:
> 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 redhat com>
> ---
>  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;

ACK


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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