Hi!
Thanks for those comments!
I fixed all the trivial stuff but I still have some questions...
2012/1/26 Guillem Jover <guillem@debian.org>:
> [ 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);
Attachment:
fix_FTBFS4Hurd.patch
Description: Binary data