Hi Arturo,
On Fri, Jul 18, 2014 at 01:08:06PM +0200, Arturo Borrero Gonzalez wrote:
> Every snprintf() call is warranteed to put the trailing \0 character, unless
> the output was truncated, ie ret == bufsiz. In case of output truncated, the
> caller must reallocate a buffer and start again.
> This \0 character is used by other functions like fputs() and fprintf() to
> know where a string ends, so they avoid adding garbage to the output.
>
> The XML/JSON event wrapper header/footer is printed for each object, meaning
> that the header is the first thing to be written to the buffer and the footer
> the last; all objects share this code path.
>
> A new helper function nft_fprintf2() is added because:
> * we need to handle the needed buffer resizing.
> * so, we can ignore about what string the header/footer are composed of.
> * nft_fprint() requires an object; the signature of the function differs.
>
> There were two cases we weren't considering in the header/footer fprintf
> functions: if the nft_event_[head|foot]er_snprintf() call returned 0 and/or if
> returned < 0. Previous to this patch, we unconditionally do a fprintf().
>
> Now with this patch, we avoid doing fprintf() if the inner snprintf() call:
> * failed (ret < 0)
> * didn't print nothing (ret == 0) to the buffer (even a \0 character)
>
> Also, we were unconditionally adding a \0 character at buf[sizeof(buf) - 1],
> meaning that if the inner snprintf() returned 0, we get sizeof(buf) - 2
> characters of unknown value.
>
> A side effect of this patch is that we avoid 2 syscall (2 fprintf() calls) per
> nft object to be printed if the header/footer is empty (ie, no events flags
> set).
>
> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@xxxxxxxxx>
> ---
> src/common.c | 14 ++------------
> src/internal.h | 3 +++
> src/utils.c | 33 +++++++++++++++++++++++++++++++++
> 3 files changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/src/common.c b/src/common.c
> index 1b600f1..2ebb7d1 100644
> --- a/src/common.c
> +++ b/src/common.c
> @@ -113,12 +113,7 @@ int nft_event_header_snprintf(char *buf, size_t size, uint32_t type,
>
> int nft_event_header_fprintf(FILE *fp, uint32_t type, uint32_t flags)
> {
> - char buf[64]; /* enough for the maximum string length above */
> -
> - nft_event_header_snprintf(buf, sizeof(buf), type, flags);
> - buf[sizeof(buf) - 1] = '\0';
> -
> - return fprintf(fp, "%s", buf);
> + return nft_fprintf2(fp, type, flags, nft_event_header_snprintf);
> }
>
> int nft_event_footer_snprintf(char *buf, size_t size, uint32_t type,
> @@ -139,10 +134,5 @@ int nft_event_footer_snprintf(char *buf, size_t size, uint32_t type,
>
> int nft_event_footer_fprintf(FILE *fp, uint32_t type, uint32_t flags)
> {
> - char buf[32]; /* enough for the maximum string length above */
> -
> - nft_event_footer_snprintf(buf, sizeof(buf), type, flags);
> - buf[sizeof(buf) - 1] = '\0';
> -
> - return fprintf(fp, "%s", buf);
> + return nft_fprintf2(fp, type, flags, nft_event_footer_snprintf);
> }
> diff --git a/src/internal.h b/src/internal.h
> index b8ed616..04c3ef9 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -136,6 +136,9 @@ int nft_get_value(enum nft_type type, void *val, void *out);
>
> #include <stdio.h>
> int nft_fprintf(FILE *fp, void *obj, uint32_t type, uint32_t flags, int (*snprintf_cb)(char *buf, size_t bufsiz, void *obj, uint32_t type, uint32_t flags));
> +int nft_fprintf2(FILE *fp, uint32_t type, uint32_t flags,
> + int (*snprintf_cb)(char *buf, size_t bufsiz, uint32_t type,
> + uint32_t flags));
> int nft_event_header_snprintf(char *buf, size_t bufsize,
> uint32_t format, uint32_t flags);
> int nft_event_header_fprintf(FILE *fp, uint32_t format, uint32_t flags);
> diff --git a/src/utils.c b/src/utils.c
> index 20a2fa3..94268c3 100644
> --- a/src/utils.c
> +++ b/src/utils.c
> @@ -219,6 +219,39 @@ out:
> return ret;
> }
>
> +int nft_fprintf2(FILE *fp, uint32_t type, uint32_t flags,
> + int (*snprintf_cb)(char *buf, size_t bufsiz, uint32_t type,
> + uint32_t flags))
> +{
> + char _buf[NFT_SNPRINTF_BUFSIZ];
> + char *buf = _buf;
> + size_t bufsiz = sizeof(_buf);
> + int ret;
> +
> + ret = snprintf_cb(buf, bufsiz, type, flags);
> + if (ret <= 0)
> + goto out;
> +
> + if (ret >= NFT_SNPRINTF_BUFSIZ) {
> + bufsiz = ret + 1;
> +
> + buf = malloc(bufsiz);
> + if (buf == NULL)
> + return -1;
> +
> + ret = snprintf_cb(buf, bufsiz, type, flags);
> + if (ret <= 0)
> + goto out;
> + }
> +
> + ret = fprintf(fp, "%s", buf);
> +out:
> + if (buf != _buf)
> + xfree(buf);
> +
> + return ret;
> +}
> +
This function looks almost exactly the same like nft_fprintf. We can
save line of code with some initial code refactorization.
So please, let us have one single nft_fprintf function. Hint: You
can generalise the snprintf_cb callback, ie.
int (*snprintf_cb)(char *buf, size_t bufsiz, uint32_t type,
uint32_t flags, void *data);
The nft_fprintf should look like:
int nft_fprintf(FILE *fp, uint32_t type, uint32_t flags, void *data, ...)
The data pointer is generic and it is interpreted by the callback
function.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
