Re: memstat: FTBFS on hurd-i386
Hi!
[ Two requests, could you disable HTML mail in your client? And could
you check it properly sets the MIME type for the attachments,
otherwise other mailers will not inline them on replies for example.
Thanks! ]
On Wed, 2012-01-25 at 14:01:35 +0100, Tanguy LE CARROUR wrote:
> I've removed the "#ifndef BUFSIZ".
> I've also re-indented the get_line() signature: remove one line-break and
> one space.
> diff -Naur old/memstat-0.9/memstat.c memstat-0.9/memstat.c
> --- old/memstat-0.9/memstat.c 2012-01-20 14:18:08.444775271 +0100
> +++ memstat-0.9/memstat.c 2012-01-25 13:54:55.375731001 +0100
> @@ -58,11 +58,34 @@
> maptab_size = maptab_size * 2 + 100;
> }
>
> +static char * get_line(FILE *f)
> +{
> + char * buf = NULL;
Small style nitpick, to match the rest of the code it seems this should
be:
static char *get_line(FILE *f)
{
char *buf = NULL;
It also seems upstream prefers buff over buf.
> + size_t buf_size = 0;
> + size_t last = 0;
> +
> + while (!feof(f)) {
> + buf_size = buf_size ? buf_size * 2 : BUFSIZ;
> + buf = realloc(buf, buf_size);
realloc might fail, you should check for that, in addition if you
don't want to leak the assigned variable has to be temporarily different
to the reallocated one.
> + if (fgets(buf + last, buf_size - last, f) == NULL) {
> + free(buf);
> + return NULL;
> + }
> + last = strlen(buf);
> + if (buf[last-1] == '\n')
Spaces around ‘-’.
> + return buf;
> + }
> +
> + return buf;
> +}
> +
> static void read_proc(void)
> {
> unsigned int nread, pid;
> unsigned long inode, lo, hi, offs;
> - char *p, major[8], minor[8], buff[PATH_MAX + 300], *path, perm[4];
> + char *p, major[8], minor[8], *path, perm[4];
> + char *buff = NULL;
> + int buff_size = 0;
size_t
> DIR *d;
> struct dirent *ent;
> FILE *f;
> @@ -85,11 +108,21 @@
> }
> if (pid == 0 || (only_pid != 0 && pid != only_pid))
> continue;
> - sprintf(buff, "/proc/%d/maps", pid);
> - f = fopen(buff, "r");
> +
> +#define FORMAT "/proc/%d/maps"
> +#define BUFF_SIZE (sizeof(FORMAT) + (sizeof(int) * 3) + 1)
> + char filename[BUFF_SIZE];
> + sprintf(filename, FORMAT, pid);
> +#undef FORMAT
> +#undef BUFF_SIZE
Given that BUFF_SIZE is used only once, it would seem cleaner to just
use it inline. Also if FORMAT was to get namespaced it could be
defined outside the function, with no need to undef it.
> + f = fopen(filename, "r");
> if (f == NULL)
> continue;
> - while (fgets(buff, sizeof(buff), f)) {
> + while (!feof(f)) {
> + buff = get_line(f);
> + if (buff == NULL)
Trailing space.
> + break;
> p = strchr(buff, '-');
> if (p)
> *p = ' ';
> @@ -128,6 +164,16 @@
> m->valid = 0;
> }
> } else {
> + buff_size = 4; /* size of the format string without "%x" expressions */
> + buff_size += strlen(major);
> + buff_size += strlen(minor);
> + buff_size += sizeof(int) * 3 + 1; // inode
> + buff_size += 1; // '\0'
Do not use C++/C99 style comments, if the code base is not using them
already and assumes GNUC/C99 base line.
> + buff = (char *)malloc(buff_size);
No need to cast the return pointer.
> + if (buff == NULL) {
> + perror("Cannot allocate memory!");
> + exit(1);
> + }
The code sometimes is returning and sometimes aborts on memory
failure, the behaviour might need unification.
> sprintf(buff, "[%s:%s]:%lu", major, minor, inode);
> m->path = strdup(buff);
> needinode = 1;
> @@ -251,17 +297,36 @@
> grand = sharedgrand = 0;
> qsort(maptab_data, maptab_fill, sizeof(struct mapping), (qcmp) sort_by_pid);
> for (offs = 0; offs < maptab_fill; offs = scan) {
> - char linkname[PATH_MAX], filename[PATH_MAX];
> + char *linkname = NULL;
> + struct stat sb;
> ssize_t len;
> int deleted = 0;
>
> pid = maptab_data[offs].pid;
> - sprintf(filename, "/proc/%d/exe", pid);
> - if ((len = readlink(filename, linkname, PATH_MAX)) == -1) {
> +
> +#define FORMAT "/proc/%d/exe"
> +#define BUFF_SIZE (sizeof(FORMAT) + (sizeof(int) * 3) + 1)
> + char filename[BUFF_SIZE];
> + sprintf(filename, FORMAT, pid);
> +#undef FORMAT
> +#undef BUFF_SIZE
Same comment as above.
> + if (lstat(filename, &sb) == -1) {
> + perror("lstat");
> + exit(EXIT_FAILURE);
> + }
> + linkname = malloc(sb.st_size + 1);
> + if (linkname == NULL) {
> + fprintf(stderr, "insufficient memory\n");
> + exit(EXIT_FAILURE);
> + }
It seems the old code was not aborting on failure, so it might make
sense to handle errors gracefully here too.
> + len = readlink(filename, linkname, sb.st_size + 1);
> +
> + if (len < 0 || len > sb.st_size) {
> fprintf(stderr, "Cannot read link information for %s\n", filename);
> deleted = 1;
> }
> - linkname[len] = '\0';
> + linkname[sb.st_size] = '\0';
> total = 0;
> for (scan = offs; scan < maptab_fill; scan++) {
> m = maptab_data + scan;
thanks,
guillem
Reply to: