Hi! Thanks for those comments! I fixed all the trivial stuff but I still have some questions...
2012/1/26 Guillem Jover <[email protected]>: > [ 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! ] I use the gmail web site. I disabled the HTML but I don't think I have a control on the attachment mime-type, detected as application/octet-stream when should be text/plain or text/patch. If someone knows how to properly do it... >> + buf = realloc(buf, buf_size); fixed. Is there a different convention than new_buff?! >> +#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. I didn't know exactly what you meant by "namespaced" (sorry, I'm a beginner)... I inlined the BUFF_SIZE and defined the FORMATs outside the function. > The code sometimes is returning and sometimes aborts on memory > failure, the behaviour might need unification. In the first function I used exit() because it's used in the original code. In the second one I tried to "gracefully" use continue but I got a warning "'scan' may not be initialized". So I added some if/else to skip some part and reach for the loop where 'scan' is initialized. But I'm not convinced... Now one stupid question related to the original code... There is a "char buffer[8192];" that will store the linkname that was allocated dynamically. Do I have to fix it? Thanks, Tanguy PS: Just in case, here is the patch 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-27 14:45:07.407903273 +0100 @@ -20,6 +20,9 @@ #include <getopt.h> #include <errno.h> +#define FMT_PROC_MAPS "/proc/%d/maps" +#define FMT_PROC_EXE "/proc/%d/exe" + /* blacklist devices that just map physical memory */ char *blacklist[] = { "/dev/mem", "/dev/dri/" @@ -58,11 +61,39 @@ maptab_size = maptab_size * 2 + 100; } +static char *get_line(FILE *f) +{ + char *buff = NULL; + char *new_buff = NULL; + size_t buff_size = 0; + size_t last = 0; + + while (!feof(f)) { + buff_size = buff_size ? buff_size * 2 : BUFSIZ; + new_buff = realloc(buff, buff_size); + if (new_buff == NULL) { + free(buff); + return NULL; + } + buff = new_buff; + if (fgets(buff + last, buff_size - last, f) == NULL) { + free(buff); + return NULL; + } + last = strlen(buff); + if (buff[last - 1] == '\n') + return buff; + } + return buff; +} + 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; + size_t buff_size = 0; DIR *d; struct dirent *ent; FILE *f; @@ -85,11 +116,15 @@ } if (pid == 0 || (only_pid != 0 && pid != only_pid)) continue; - sprintf(buff, "/proc/%d/maps", pid); - f = fopen(buff, "r"); + char filename[sizeof(FMT_PROC_MAPS) + (sizeof(int) * 3) + 1]; + sprintf(filename, FMT_PROC_MAPS, pid); + f = fopen(filename, "r"); if (f == NULL) continue; - while (fgets(buff, sizeof(buff), f)) { + while (!feof(f)) { + buff = get_line(f); + if (buff == NULL) + break; p = strchr(buff, '-'); if (p) *p = ' '; @@ -97,9 +132,12 @@ if (p) *p = ' '; path = NULL; - if ((strlen(buff) == 10) && (strcmp(buff, " (deleted)") == 0)) + if ((strlen(buff) == 10) && (strcmp(buff, " (deleted)") == 0)) { + free(buff); continue; + } nread = sscanf(buff, "%lx %lx %4s %lx %s %s %lu %as", &lo, &hi, perm, &offs, major, minor, &inode, &path); + free(buff); if (nread < 7) { fprintf(stderr, "I don't recognize format of /proc/%d/maps. (nread=%d)\n", pid, nread); exit(1); @@ -128,6 +166,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' */ + buff = malloc(buff_size); + if (buff == NULL) { + perror("Cannot allocate memory!"); + exit(1); + } sprintf(buff, "[%s:%s]:%lu", major, minor, inode); m->path = strdup(buff); needinode = 1; @@ -251,17 +299,35 @@ 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]; - ssize_t len; + char *linkname = NULL; + struct stat sb; + ssize_t len = -1; int deleted = 0; pid = maptab_data[offs].pid; - sprintf(filename, "/proc/%d/exe", pid); - if ((len = readlink(filename, linkname, PATH_MAX)) == -1) { + char filename[sizeof(FMT_PROC_EXE) + (sizeof(int) * 3) + 1]; + sprintf(filename, FMT_PROC_EXE, pid); + if (lstat(filename, &sb) == -1) { + perror("lstat"); + deleted = 1; + } + else { + linkname = malloc(sb.st_size + 1); + if (linkname == NULL) { + fprintf(stderr, "insufficient memory\n"); + deleted = 1; + } + else { + 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'; + else { + linkname[sb.st_size] = '\0'; + } total = 0; for (scan = offs; scan < maptab_fill; scan++) { m = maptab_data + scan; @@ -277,6 +343,7 @@ printline(buffer); grand += total; } + free(linkname); } qsort(maptab_data, maptab_fill, sizeof(struct mapping), (qcmp) sort_by_inode);
fix_FTBFS4Hurd.patch
Description: Binary data
