[Devel] [PATCH] vzlist: dynamically allocated output buffer
Kir Kolyshkin
kir at openvz.org
Mon Nov 24 13:25:26 PST 2014
On 11/24/2014 11:20 AM, Андрей Пушкарь wrote:
> Thank you. Few more questions:
> 1) declaring variables almost everywhere we want is a part of C99
> standard, as I understand. Could you please explain what is wrong with
> this practice?
It's just that the current code doesn't do it, and I don't like to
change that now.
> 2) I'm sorry, but I didn't get what you mean by the last line of your
> letter.
Your code accidentally reverted my recent commit. You can
use git pull --rebase to avoid that, and you should always check your
patch before sending to make sure there are no changes you
don't intend to have.
>
> Thank you.
>
> 2014-11-24 22:12 GMT+03:00 Kir Kolyshkin <kir at openvz.org
> <mailto:kir at openvz.org>>:
>
> Thanks, this looks better than the previous version.
>
> Please see my comments inline.
>
> Please send a new version to devel@, cc: kir@ (one email, not two
> separate ones).
>
>
> On 11/24/2014 09:46 AM, 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.
>
>
> 1 Please make the description fit in 80 columns.
>
> 2 Please briefly describe the test case(s) you were using to check
> the patch.
>
>
>
> https://bugzilla.openvz.org/2999
>
> Reported-by: Michal Grzedzicki <lazy at iq.pl <mailto:lazy at iq.pl>>
> Signed-off-by: Andrey Pushkar <andrey.s.pushkar at gmail.com
> <mailto:andrey.s.pushkar at gmail.com>>
> ---
> src/vzlist.c | 118
> +++++++++++++++++++++++++++++++++-------------------------
> 1 files changed, 67 insertions(+), 51 deletions(-)
>
> diff --git a/src/vzlist.c b/src/vzlist.c
> index 0b66f75..38bb7ae 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;
> +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_snprintf(const char *format, ...);
>
>
> 1 This is no longer snprintf-like function, please rename. You can
> call it xprintf or something.
>
> 2 Please use __attribute__ (__format__ ( __printf__, 1, 2))) here,
> so C compiler can
> check the arguments. See
> https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
>
> +static void *x_realloc(void *ptr, int size);
> +
>
>
> I think you don't need this prototype here.
>
>
> /* 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_snprintf("%" #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_snprintf("%-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_snprintf("%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_snprintf("%-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_snprintf("%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_snprintf("%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_snprintf("%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_snprintf("%.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_snprintf("%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_snprintf("%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_snprintf(fmt "s", "-");
> \
> else \
> - p_buf += snprintf(p_buf, e_buf - p_buf, \
> - fmt "d", i); \
> + p_buf += x_snprintf(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_snprintf(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_snprintf("%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_snprintf("%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_snprintf("%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_snprintf("%5s", "-");
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf,
> - "%5d", p->cpunum);
> + p_buf += x_snprintf("%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_snprintf("%-6s",
> layout2str(p->layout));
> }
> 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_snprintf("%-15s", str);
> if (!is_last_field)
> r = 15;
> p_buf += r;
> @@ -440,9 +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_snprintf("%10s", "-");
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf, "%10lu",
> + p_buf += x_snprintf("%10lu",
> res[index]);
> }
> @@ -493,10 +481,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_snprintf("%10s", "-");
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf, "%10lu",
> - res[index]);
> + p_buf += x_snprintf("%10lu", res[index]);
> }
> #define PRINT_DQ(name) \
> @@ -517,10 +504,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_snprintf("%6s", "-");
> else
> - p_buf += snprintf(p_buf, e_buf - p_buf, "%6g",
> - p->vm_overcommit);
> + p_buf += x_snprintf("%6g", p->vm_overcommit);
> }
> /* Sort functions */
> @@ -790,6 +776,32 @@ static void *x_realloc(void *ptr, int size)
> return tmp;
> }
> +static int x_snprintf(const char *format, ...)
> +{
> + va_list arglist, tmp_arglist;
> + va_start(arglist, format);
> + va_start(tmp_arglist, format);
> +
> + int 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;
> + char *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;
> + }
> +
> + int ret = vsnprintf(p_buf, e_buf - p_buf, format,
> arglist);
>
>
> 1 Don't declare variables in the middle of the function
>
> 2 You don't need to print two times in fact -- only if you need to
> resize the buffer.
>
> + va_end(arglist);
> + va_end(tmp_arglist);
> + return ret;
> +}
> +
> static void usage()
> {
> printf(
> @@ -843,8 +855,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_snprintf(field_names[f].hdr_fmt,
> field_names[f].hdr);
> if (p_buf >= e_buf)
> break;
> if (p->next != NULL)
> @@ -1673,7 +1684,7 @@ static int get_ves_cpu()
> }
> if (id && !veid) {
> rate = rint((double)rate * 100 / 1024);
> - weight = rint((double)MAXCPUUNITS /
> weight);
> + weight = MAXCPUUNITS / weight;
>
>
> Uh oh. Please rebase your stuff properly.
>
>
> update_cpu(id, rate, weight);
> }
> }
> @@ -1911,6 +1922,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 +2030,7 @@ int main(int argc, char **argv)
> free(name_pattern);
> free(desc_pattern);
> free(f_order);
> + free(g_buf);
> return 0;
> }
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/devel/attachments/20141124/00cd3aaa/attachment-0001.html>
More information about the Devel
mailing list