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 <[email protected]>
Signed-off-by: Andrey Pushkar <[email protected]>
---
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;
}
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
