[Devel] [PATCH] vzlist: dynamically allocated output buffer
Kir Kolyshkin
kir at openvz.org
Thu Dec 4 17:10:57 PST 2014
Sorry for the delay. This looks way better than before.
Please fix according to the comments inline and I will review it again.
Kir.
On 12/01/2014 12:49 PM, Andrey Pushkar wrote:
> As pointed out in bug #2999, vzlist -o ip segfaults if the output string
> (IP addresses list in this case) exceeds the limit of 4096 characters.
> The issue is caused by static memory allocation, which has been changed
> to dynamic in this commit: a snprintf wrapper that implements automatic
> buffer resizing has been introduced.
>
> Test cases: as adding lots of IP addresses takes pretty much time,
> the constant of 4096 bytes (primary buffer size) was reduced to the smaller
> values, and several sets of IP addresses (with growing number of items and,
> therefore, growing output string length) were added to the VE and then sent
> to console via modified vzlist.
> Also, valgrind doesn't show any new memory leaks comparing to vanilla
> version.
>
> https://bugzilla.openvz.org/2999
>
> Reported-by: Michal Grzedzicki <lazy at iq.pl>
> Signed-off-by: Andrey Pushkar <andrey.s.pushkar at gmail.com>
> ---
> src/vzlist.c | 121 +++++++++++++++++++++++++++++++++------------------------
> 1 files changed, 70 insertions(+), 51 deletions(-)
>
> diff --git a/src/vzlist.c b/src/vzlist.c
> index 0b66f75..fe816a6 100644
> --- a/src/vzlist.c
> +++ b/src/vzlist.c
> @@ -21,6 +21,7 @@
> #include <stdlib.h>
> #include <ctype.h>
> #include <string.h>
> +#include <stdarg.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <sys/socket.h>
> @@ -51,9 +52,10 @@
> static struct Cveinfo *veinfo = NULL;
> static int n_veinfo = 0;
>
> -static char g_buf[4096] = "";
> -static char *p_buf = g_buf;
> -static char *e_buf = g_buf + sizeof(g_buf) - 1;
> +static char *g_buf = NULL;
> +static long g_buf_size = 4096;
There is no need to have this variable, as we have g_buf and e_buf,
we can always calculate buffer length, say in x_printf() you can have:
int bufsize = e_buf - g_buf;
Alternatively, you can eliminate e_buf and use g_buf_size.
In any case, don't have both as it makes things more complicated
than it should be.
> +static char *p_buf = NULL;
> +static char *e_buf = NULL;
> static char *host_pattern = NULL;
> static char *name_pattern = NULL;
> static char *desc_pattern = NULL;
> @@ -90,6 +92,9 @@ static inline int get_run_ve(int update)
> #define get_run_ve get_run_ve_proc
> #endif
>
> +static int x_printf(const char *format, ...)
> + __attribute__ ((format (printf, 1, 2)));
> +
> /* Print functions */
> static void print_json_str(const char *s)
> {
> @@ -110,8 +115,7 @@ static void print_ ## funcname(struct Cveinfo *p, int index) \
> \
> if (p->fieldname != NULL) \
> str = p->fieldname; \
> - r = snprintf(p_buf, e_buf - p_buf, \
> - "%" #length "s", str); \
> + r = x_printf("%" #length "s", str); \
> if (!is_last_field) \
> r = abs(length); \
> p_buf += r; \
> @@ -177,7 +181,7 @@ static void print_strlist(char *list)
> if ((ch = strchr(str, ' ')) != NULL)
> *ch = 0;
> }
> - r = snprintf(p_buf, e_buf - p_buf, "%-15s", str);
> + r = x_printf("%-15s", str);
> if (!is_last_field)
> r = 15;
> p_buf += r;
> @@ -203,8 +207,7 @@ static void print_veid(struct Cveinfo *p, int index)
> if (fmt_json)
> printf("%d", p->veid);
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf,
> - "%10d", p->veid);
> + p_buf += x_printf("%10d", p->veid);
> }
>
> static void print_smart_name(struct Cveinfo *p, int index)
> @@ -220,8 +223,7 @@ static void print_status(struct Cveinfo *p, int index)
> if (fmt_json)
> print_json_str(ve_status[p->status]);
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf,
> - "%-9s", ve_status[p->status]);
> + p_buf += x_printf("%-9s", ve_status[p->status]);
> }
>
> static void print_laverage(struct Cveinfo *p, int index)
> @@ -230,8 +232,7 @@ static void print_laverage(struct Cveinfo *p, int index)
> if (fmt_json)
> printf("null");
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf,
> - "%14s", "-");
> + p_buf += x_printf("%14s", "-");
> }
> else {
> if (fmt_json)
> @@ -240,8 +241,7 @@ static void print_laverage(struct Cveinfo *p, int index)
> p->cpustat->la[1],
> p->cpustat->la[2]);
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf,
> - "%1.2f/%1.2f/%1.2f",
> + p_buf += x_printf("%1.2f/%1.2f/%1.2f",
> p->cpustat->la[0],
> p->cpustat->la[1],
> p->cpustat->la[2]);
> @@ -257,8 +257,7 @@ static void print_uptime(struct Cveinfo *p, int index)
> }
>
> if (p->cpustat == NULL)
> - p_buf += snprintf(p_buf, e_buf - p_buf,
> - "%15s", "-");
> + p_buf += x_printf("%15s", "-");
> else
> {
> unsigned int days, hours, min, secs;
> @@ -272,8 +271,7 @@ static void print_uptime(struct Cveinfo *p, int index)
> (60ull * min + 60ull * 60 * hours +
> 60ull * 60 * 24 * days));
>
> - p_buf += snprintf(p_buf, e_buf - p_buf,
> - "%.3dd%.2dh:%.2dm:%.2ds",
> + p_buf += x_printf("%.3dd%.2dh:%.2dm:%.2ds",
> days, hours, min, secs);
> }
> }
> @@ -285,15 +283,13 @@ static void print_cpu ## name(struct Cveinfo *p, int index) \
> if (fmt_json) \
> printf("null"); \
> else \
> - p_buf += snprintf(p_buf, e_buf - p_buf, \
> - "%7s", "-"); \
> + p_buf += x_printf("%7s", "-"); \
> } \
> else { \
> if (fmt_json) \
> printf("%lu", p->cpu->name[index]); \
> else \
> - p_buf += snprintf(p_buf, e_buf - p_buf, \
> - "%7lu", p->cpu->name[index]); \
> + p_buf += x_printf("%7lu", p->cpu->name[index]); \
> } \
> }
>
> @@ -311,11 +307,9 @@ static void print_io ## name(struct Cveinfo *p, int index) \
> } \
> \
> if (i < min) \
> - p_buf += snprintf(p_buf, e_buf - p_buf, \
> - fmt "s", "-"); \
> + p_buf += x_printf(fmt "s", "-"); \
> else \
> - p_buf += snprintf(p_buf, e_buf - p_buf, \
> - fmt "d", i); \
> + p_buf += x_printf(fmt "d", i); \
> }
>
> PRINT_IO(prio, "%3", 0)
> @@ -328,8 +322,7 @@ static void print_bool(const char *fmt, int val)
> if (fmt_json)
> printf(val ? "true" : "false");
> else
> - p_buf += snprintf(p_buf, e_buf-p_buf,
> - fmt, val ? "yes" : "no");
> + p_buf += x_printf(fmt, val ? "yes" : "no");
> }
>
> static void print_onboot(struct Cveinfo *p, int index)
> @@ -341,7 +334,7 @@ static void print_onboot(struct Cveinfo *p, int index)
> if (fmt_json)
> printf("null");
> else
> - p_buf += snprintf(p_buf, e_buf-p_buf, "%6s", "-");
> + p_buf += x_printf("%6s", "-");
> }
>
> static void print_bootorder(struct Cveinfo *p, int index)
> @@ -350,15 +343,13 @@ static void print_bootorder(struct Cveinfo *p, int index)
> if (fmt_json)
> printf("null");
> else
> - p_buf += snprintf(p_buf, e_buf-p_buf,
> - "%10s", "-");
> + p_buf += x_printf("%10s", "-");
> }
> else {
> if (fmt_json)
> printf("%lu", p->bootorder[index]);
> else
> - p_buf += snprintf(p_buf, e_buf-p_buf,
> - "%10lu", p->bootorder[index]);
> + p_buf += x_printf("%10lu", p->bootorder[index]);
> }
> }
>
> @@ -370,11 +361,9 @@ static void print_cpunum(struct Cveinfo *p, int index)
> }
>
> if (p->cpunum <= 0)
> - p_buf += snprintf(p_buf, e_buf - p_buf,
> - "%5s", "-");
> + p_buf += x_printf("%5s", "-");
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf,
> - "%5d", p->cpunum);
> + p_buf += x_printf("%5d", p->cpunum);
> }
>
> static const char *layout2str(int layout)
> @@ -394,8 +383,7 @@ static void print_layout(struct Cveinfo *p, int index)
> if (fmt_json)
> print_json_str(layout2str(p->layout));
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf,
> - "%-6s", layout2str(p->layout));
> + p_buf += x_printf("%-6s", layout2str(p->layout));
tab instead of space here
> }
>
> static void print_features(struct Cveinfo *p, int index)
> @@ -409,7 +397,7 @@ static void print_features(struct Cveinfo *p, int index)
>
> features_mask2str(p->features_mask, p->features_known,
> ",", str, sizeof(str));
> - r = snprintf(p_buf, e_buf - p_buf, "%-15s", str);
> + r = x_printf("%-15s", str);
> if (!is_last_field)
> r = 15;
> p_buf += r;
> @@ -440,10 +428,9 @@ static void print_ubc(struct Cveinfo *p, size_t res_off, int index)
> }
>
> if (!res || (!running && (index == 0 || index == 1 || index == 4)))
> - p_buf += snprintf(p_buf, e_buf - p_buf, "%10s", "-");
> + p_buf += x_printf("%10s", "-");
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf, "%10lu",
> - res[index]);
> + p_buf += x_printf("%10lu", res[index]);
> }
>
> #ifndef offsetof
> @@ -493,10 +480,9 @@ static void print_dq(struct Cveinfo *p, size_t res_off, int index)
> }
>
> if (!res || (index == 0 && res[index] == 0))
> - p_buf += snprintf(p_buf, e_buf - p_buf, "%10s", "-");
> + p_buf += x_printf("%10s", "-");
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf, "%10lu",
> - res[index]);
> + p_buf += x_printf("%10lu", res[index]);
> }
>
> #define PRINT_DQ(name) \
> @@ -517,10 +503,9 @@ static void print_vm_overcommit(struct Cveinfo *p, int index)
> }
>
> if (p->vm_overcommit == 0)
> - p_buf += snprintf(p_buf, e_buf - p_buf, "%6s", "-");
> + p_buf += x_printf("%6s", "-");
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf, "%6g",
> - p->vm_overcommit);
> + p_buf += x_printf("%6g", p->vm_overcommit);
> }
>
> /* Sort functions */
> @@ -790,6 +775,36 @@ static void *x_realloc(void *ptr, int size)
> return tmp;
> }
>
> +static int x_printf(const char *format, ...)
> +{
> + int write_len = 0;
> + char *tmp = NULL;
> +
> + va_list arglist, tmp_arglist;
I suggest you only use one arglist. As va_start() man page says,
> Multiple traversals of the list, each bracketed by va_start() and
va_end() are possible.
So you just do another va_start() and va_end() inside that if statement
that calls
> + va_start(arglist, format);
> + va_start(tmp_arglist, format);
> +
^^^ extra tabs in here
> + write_len = vsnprintf(p_buf, e_buf - p_buf, format, tmp_arglist);
> +
> + write_len++; // vsnprintf doesn't count the last \0
> +
> + if (e_buf - p_buf <= write_len)
> + {
> + g_buf_size = write_len >= g_buf_size * 2 ? write_len : g_buf_size * 2;
> + tmp = (char *)x_realloc(g_buf, sizeof(char) * g_buf_size);
> +
> + p_buf = tmp + (p_buf - g_buf);
> + g_buf = tmp;
> + e_buf = g_buf + g_buf_size - 1;
> +
... and here.
I use the following rules in my ~.vimrc to highlight such errors (also see
if my code is over 80 columns), might be helpful if you also use vim:
let c_space_errors = 1
let python_highlight_space_errors = 1
highlight FormatError ctermbg=darkred guibg=darkred
match FormatError /\s\+$\|\ \+\t\|\%80v.\|\ \{8\}/
autocmd FileType diff match FormatError /\%>1v\(\s\+$\|\ \+\t\|\%81v.\|\
\{8\}\)/
autocmd FileType python match FormatError /\t\|\s\+$\|\%80v./
> + write_len = vsnprintf(p_buf, e_buf - p_buf, format, arglist);
> + }
> +
> + va_end(arglist);
> + va_end(tmp_arglist);
> + return write_len;
> +}
> +
> static void usage()
> {
> printf(
> @@ -843,8 +858,7 @@ static void print_hdr()
>
> for (p = g_field_order; p != NULL; p = p->next) {
> f = p->order;
> - p_buf += snprintf(p_buf, e_buf - p_buf,
> - field_names[f].hdr_fmt, field_names[f].hdr);
> + p_buf += x_printf(field_names[f].hdr_fmt, field_names[f].hdr);
> if (p_buf >= e_buf)
> break;
> if (p->next != NULL)
> @@ -1911,6 +1925,10 @@ int main(int argc, char **argv)
> char *ep, *p;
> int veid, c;
>
> + g_buf = (char *)malloc(sizeof(char) * g_buf_size);
> + p_buf = g_buf;
> + e_buf = g_buf + g_buf_size - 1;
> +
> while (1) {
> int option_index = -1;
> c = getopt_long(argc, argv, "HtSanjN:h:d:o:s:Le1",
> @@ -2015,6 +2033,7 @@ int main(int argc, char **argv)
> free(name_pattern);
> free(desc_pattern);
> free(f_order);
> + free(g_buf);
>
> return 0;
> }
More information about the Devel
mailing list