On 03.01.2007 [10:27:19 +1100], David Gibson wrote:
> On Tue, Jan 02, 2007 at 03:03:00PM -0800, Nishanth Aravamudan wrote:
> > Some of our error and debugging paths are missing newlines, which makes
> > for confusing output when things are misconfigured. Make sure all
> > ERROR and WARNING in the library is properly terminated.
>
> Actually, I think the right way to fix this is to remove \n from any
> ERROR() and WARNING() macros that do have then, and add them back in
> the macro itself. I'm pretty sure the reason I've made these errors
> is because I was assuming that ERROR() and WARNING() added the \n,
> like CONFIG() and FAIL(), which in turn work that way because perror()
> does.
I disagree with this, just because it means we, for instance, in the
case with my other patch to *remove* a newline, will now need to provide
another set of non-newlined macros...
For what it's worth, though, here is the patch that does what you
suggest, it's a lot of churn for not much gain, when my other patch,
along with the flush() version of my debug change patch, seem to be
sufficient?
diff --git a/elflink.c b/elflink.c
index b621a3a..470fb99 100644
--- a/elflink.c
+++ b/elflink.c
@@ -254,7 +254,7 @@ static void parse_phdrs(Elf_Ehdr *ehdr)
if (htlb_num_segs >= MAX_HTLB_SEGS) {
ERROR("Executable has too many segments marked for "
- "hugepage (max %d)\n", MAX_HTLB_SEGS);
+ "hugepage (max %d)", MAX_HTLB_SEGS);
htlb_num_segs = 0;
return;
}
@@ -271,7 +271,7 @@ static void parse_phdrs(Elf_Ehdr *ehdr)
DEBUG("Hugepage segment %d "
"(phdr %d): %#0lx-%#0lx (filesz=%#0lx) "
- "(prot = %#0x)\n",
+ "(prot = %#0x)",
htlb_num_segs, i, vaddr, vaddr+memsz, filesz, prot);
htlb_seg_table[htlb_num_segs].vaddr = (void *)vaddr;
@@ -306,7 +306,7 @@ static int find_or_create_share_path(void)
/* Given an explicit path */
if (hugetlbfs_test_path(env) != 0) {
ERROR("HUGETLB_SHARE_PATH %s is not on a hugetlbfs"
- " filesystem\n", share_path);
+ " filesystem", share_path);
return -1;
}
assemble_path(share_path, "%s", env);
@@ -325,7 +325,7 @@ static int find_or_create_share_path(void)
/* Check the share directory is sane */
ret = lstat(share_path, &sb);
if (ret != 0) {
- ERROR("Couldn't stat() %s: %s\n", share_path, strerror(errno));
+ ERROR("Couldn't stat() %s: %s", share_path, strerror(errno));
return -1;
}
@@ -335,13 +335,13 @@ static int find_or_create_share_path(void)
}
if (sb.st_uid != getuid()) {
- ERROR("%s has wrong owner (uid=%d instead of %d)\n",
+ ERROR("%s has wrong owner (uid=%d instead of %d)",
share_path, sb.st_uid, getuid());
return -1;
}
if (sb.st_mode & (S_IWGRP | S_IWOTH)) {
- ERROR("%s has bad permissions 0%03o\n",
+ ERROR("%s has bad permissions 0%03o",
share_path, sb.st_mode);
return -1;
}
@@ -359,7 +359,7 @@ static void check_bss(unsigned long *start, unsigned long
*end)
for (addr = start; addr < end; addr++) {
if (*addr != 0)
- WARNING("Non-zero BSS data @ %p: %lx\n", addr, *addr);
+ WARNING("Non-zero BSS data @ %p: %lx", addr, *addr);
}
}
@@ -388,13 +388,13 @@ static int get_shared_file_name(struct seg_info
*htlb_seg_info, char *file_path)
ret = readlink("/proc/self/exe", binary, PATH_MAX);
if (ret < 0) {
ERROR("shared_file: readlink() on /proc/self/exe "
- "failed: %s\n", strerror(errno));
+ "failed: %s", strerror(errno));
return -1;
}
binary2 = basename(binary);
if (!binary2) {
- ERROR("shared_file: basename() on %s failed: %s\n",
+ ERROR("shared_file: basename() on %s failed: %s",
binary, strerror(errno));
return -1;
}
@@ -419,7 +419,7 @@ static int find_dynamic(Elf_Dyn **dyntab)
*dyntab = (Elf_Dyn *)phdr[i].p_vaddr;
return 0;
} else {
- DEBUG("No dynamic segment found\n");
+ DEBUG("No dynamic segment found");
return -1;
}
}
@@ -437,11 +437,11 @@ static int find_tables(Elf_Dyn *dyntab, Elf_Sym **symtab,
char **strtab)
}
if (!*symtab) {
- DEBUG("No symbol table found\n");
+ DEBUG("No symbol table found");
return -1;
}
if (!*strtab) {
- DEBUG("No string table found\n");
+ DEBUG("No string table found");
return -1;
}
return 0;
@@ -458,7 +458,7 @@ static int find_numsyms(Elf_Sym *symtab, char *strtab)
* must maintain this assumption or this code will break.
*/
if ((void *)strtab <= (void *)symtab) {
- DEBUG("Could not calculate dynamic symbol table size\n");
+ DEBUG("Could not calculate dynamic symbol table size");
return -1;
}
return ((void *)strtab - (void *)symtab) / sizeof(Elf_Sym);
@@ -556,7 +556,7 @@ static void get_extracopy(struct seg_info *seg, void
**extra_start,
}
bail:
- DEBUG("Unable to perform minimal copy\n");
+ DEBUG("Unable to perform minimal copy");
bail2:
*extra_start = start_orig;
*extra_end = end_orig;
@@ -588,7 +588,7 @@ static int prepare_segment(struct seg_info *seg)
p = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, seg->fd, 0);
if (p == MAP_FAILED) {
- ERROR("Couldn't map hugepage segment to copy data: %s\n",
+ ERROR("Couldn't map hugepage segment to copy data: %s",
strerror(errno));
return -1;
}
@@ -600,17 +600,17 @@ static int prepare_segment(struct seg_info *seg)
* be contained in the filesz portion of the segment.
*/
- DEBUG("Mapped hugeseg at %p. Copying %#0lx bytes from %p...\n",
+ DEBUG("Mapped hugeseg at %p. Copying %#0lx bytes from %p...",
p, seg->filesz, seg->vaddr);
memcpy(p, seg->vaddr, seg->filesz);
- DEBUG_CONT("done\n");
+ DEBUG_CONT("done");
if (extra_end > extra_start) {
- DEBUG("Copying extra %#0lx bytes from %p...\n",
+ DEBUG("Copying extra %#0lx bytes from %p...",
(unsigned long)(extra_end - extra_start), extra_start);
gap = extra_start - (seg->vaddr + seg->filesz);
memcpy((p + seg->filesz + gap), extra_start, (extra_end -
extra_start));
- DEBUG_CONT("done\n");
+ DEBUG_CONT("done");
}
munmap(p, size);
@@ -678,12 +678,12 @@ static int find_or_prepare_shared_file(struct seg_info
*htlb_seg_info)
ret = unlink(tmp_path);
if (ret != 0)
ERROR("shared_file: unable to clean up"
- " unneeded file %s: %s\n",
+ " unneeded file %s: %s",
tmp_path, strerror(errno));
close(fdx);
} else if (errnox != EEXIST) {
WARNING("shared_file: Unexpected failure on
exclusive"
- " open of %s: %s\n", tmp_path,
+ " open of %s: %s", tmp_path,
strerror(errnox));
}
htlb_seg_info->fd = fds;
@@ -694,22 +694,22 @@ static int find_or_prepare_shared_file(struct seg_info
*htlb_seg_info)
/* It's our job to prepare */
if (errnos != ENOENT)
WARNING("shared_file: Unexpected failure on"
- " shared open of %s: %s\n", final_path,
+ " shared open of %s: %s", final_path,
strerror(errnos));
htlb_seg_info->fd = fdx;
- DEBUG("Got unpopulated shared fd -- Preparing\n");
+ DEBUG("Got unpopulated shared fd -- Preparing");
ret = prepare_segment(htlb_seg_info);
if (ret < 0)
goto fail;
- DEBUG("Prepare succeeded\n");
+ DEBUG("Prepare succeeded");
/* move to permanent location */
ret = rename(tmp_path, final_path);
if (ret != 0) {
ERROR("shared_file: unable to rename %s"
- " to %s: %s\n", tmp_path, final_path,
+ " to %s: %s", tmp_path, final_path,
strerror(errno));
goto fail;
}
@@ -728,7 +728,7 @@ static int find_or_prepare_shared_file(struct seg_info
*htlb_seg_info)
ret = unlink(tmp_path);
if (ret != 0)
ERROR("shared_file: Unable to clean up temp file %s on"
- " failure: %s\n", tmp_path, strerror(errno));
+ " failure: %s", tmp_path, strerror(errno));
close(fdx);
}
if (fds > 0)
@@ -759,7 +759,7 @@ static int obtain_prepared_file(struct seg_info
*htlb_seg_info)
if (ret == 0)
return 0;
/* but, fall through to unlinked files, if sharing fails */
- DEBUG("Falling back to unlinked files\n");
+ DEBUG("Falling back to unlinked files");
}
fd = hugetlbfs_unlinked_fd();
if (fd < 0)
@@ -768,10 +768,10 @@ static int obtain_prepared_file(struct seg_info
*htlb_seg_info)
ret = prepare_segment(htlb_seg_info);
if (ret < 0) {
- DEBUG("Failed to prepare segment\n");
+ DEBUG("Failed to prepare segment");
return -1;
}
- DEBUG("Prepare succeeded\n");
+ DEBUG("Prepare succeeded");
return 0;
}
@@ -827,41 +827,44 @@ static int check_env(void)
env = getenv("HUGETLB_ELFMAP");
if (env && (strcasecmp(env, "no") == 0)) {
DEBUG("HUGETLB_ELFMAP=%s, not attempting to remap program "
- "segments\n", env);
+ "segments", env);
return -1;
}
env = getenv("LD_PRELOAD");
if (env && strstr(env, "libhugetlbfs")) {
- ERROR("LD_PRELOAD is incompatible with segment remapping\n");
- ERROR("Segment remapping has been DISABLED\n");
+ ERROR("LD_PRELOAD is incompatible with segment remapping");
+ ERROR("Segment remapping has been DISABLED");
return -1;
}
env = getenv("HUGETLB_MINIMAL_COPY");
if (env && (strcasecmp(env, "no") == 0)) {
DEBUG("HUGETLB_MINIMAL_COPY=%s, disabling filesz copy "
- "optimization\n", env);
+ "optimization", env);
minimal_copy = 0;
}
env = getenv("HUGETLB_SHARE");
if (env)
sharing = atoi(env);
- DEBUG("HUGETLB_SHARE=%d, sharing ", sharing);
if (sharing == 2) {
- DEBUG_CONT("enabled for all segments\n");
+ DEBUG("HUGETLB_SHARE=%d, sharing enabled for all"
+ " segments", sharing);
} else {
if (sharing == 1) {
- DEBUG_CONT("enabled for only read-only segments\n");
+ DEBUG("HUGETLB_SHARE=%d, sharing enabled for"
+ " only read-only segments",
+ sharing);
} else {
- DEBUG_CONT("disabled\n");
+ DEBUG("HUGETLB_SHARE=%d, sharing disabled",
+ sharing);
}
}
env = getenv("HUGETLB_DEBUG");
if (env) {
- DEBUG("HUGETLB_DEBUG=%s, enabling extra checking\n", env);
+ DEBUG("HUGETLB_DEBUG=%s, enabling extra checking", env);
__debug = 1;
}
@@ -876,7 +879,7 @@ static void __attribute__ ((constructor))
setup_elflink(void)
if (! ehdr) {
DEBUG("Couldn't locate __executable_start, "
- "not attempting to remap segments\n");
+ "not attempting to remap segments");
return;
}
@@ -886,7 +889,7 @@ static void __attribute__ ((constructor))
setup_elflink(void)
parse_phdrs(ehdr);
if (htlb_num_segs == 0) {
- DEBUG("Executable is not linked for hugepage segments\n");
+ DEBUG("Executable is not linked for hugepage segments");
return;
}
@@ -901,7 +904,7 @@ static void __attribute__ ((constructor))
setup_elflink(void)
for (i = 0; i < htlb_num_segs; i++) {
ret = obtain_prepared_file(&htlb_seg_table[i]);
if (ret < 0) {
- DEBUG("Failed to setup hugetlbfs file\n");
+ DEBUG("Failed to setup hugetlbfs file");
return;
}
}
diff --git a/hugeutils.c b/hugeutils.c
index 9af92b9..853f161 100644
--- a/hugeutils.c
+++ b/hugeutils.c
@@ -62,7 +62,7 @@ static long read_meminfo(const char *tag)
fd = open("/proc/meminfo", O_RDONLY);
if (fd < 0) {
- ERROR("Couldn't open /proc/meminfo (%s)\n", strerror(errno));
+ ERROR("Couldn't open /proc/meminfo (%s)", strerror(errno));
return -1;
}
@@ -70,11 +70,11 @@ static long read_meminfo(const char *tag)
readerr = errno;
close(fd);
if (len < 0) {
- ERROR("Error reading /proc/meminfo (%s)\n", strerror(readerr));
+ ERROR("Error reading /proc/meminfo (%s)", strerror(readerr));
return -1;
}
if (len == sizeof(buf)) {
- ERROR("/proc/meminfo is too large\n");
+ ERROR("/proc/meminfo is too large");
return -1;
}
buf[len] = '\0';
@@ -86,7 +86,7 @@ static long read_meminfo(const char *tag)
p += strlen(tag);
val = strtol(p, &q, 0);
if (! isspace(*q)) {
- ERROR("Couldn't parse /proc/meminfo value\n");
+ ERROR("Couldn't parse /proc/meminfo value");
return -1;
}
@@ -158,11 +158,11 @@ const char *hugetlbfs_find_path(void)
if (tmp) {
err = hugetlbfs_test_path(tmp);
if (err < 0) {
- ERROR("Can't statfs() \"%s\" (%s)\n",
+ ERROR("Can't statfs() \"%s\" (%s)",
tmp, strerror(errno));
return NULL;
} else if (err == 0) {
- ERROR("\"%s\" is not a hugetlbfs mount\n", tmp);
+ ERROR("\"%s\" is not a hugetlbfs mount", tmp);
return NULL;
}
strncpy(htlb_mount, tmp, sizeof(htlb_mount)-1);
@@ -174,7 +174,7 @@ const char *hugetlbfs_find_path(void)
if (fd < 0) {
fd = open("/etc/mtab", O_RDONLY);
if (fd < 0) {
- ERROR("Couldn't open /proc/mounts or /etc/mtab (%s)\n",
+ ERROR("Couldn't open /proc/mounts or /etc/mtab (%s)",
strerror(errno));
return NULL;
}
@@ -184,11 +184,11 @@ const char *hugetlbfs_find_path(void)
readerr = errno;
close(fd);
if (len < 0) {
- ERROR("Error reading mounts (%s)\n", strerror(errno));
+ ERROR("Error reading mounts (%s)", strerror(errno));
return NULL;
}
if (len >= sizeof(buf)) {
- ERROR("/proc/mounts is too long\n");
+ ERROR("/proc/mounts is too long");
return NULL;
}
buf[sizeof(buf)-1] = '\0';
@@ -210,7 +210,7 @@ const char *hugetlbfs_find_path(void)
}
WARNING("Could not find hugetlbfs mount point in /proc/mounts. "
- "Is it mounted?\n");
+ "Is it mounted?");
return NULL;
}
@@ -234,7 +234,7 @@ int hugetlbfs_unlinked_fd(void)
fd = mkstemp64(name);
if (fd < 0) {
- ERROR("mkstemp() failed: %s\n", strerror(errno));
+ ERROR("mkstemp() failed: %s", strerror(errno));
return -1;
}
diff --git a/libhugetlbfs_internal.h b/libhugetlbfs_internal.h
index 10b02fe..bf95725 100644
--- a/libhugetlbfs_internal.h
+++ b/libhugetlbfs_internal.h
@@ -31,20 +31,36 @@
extern int __hugetlbfs_verbose;
#define ERROR(...) \
- if (__hugetlbfs_verbose >= 1) \
- fprintf(stderr, "libhugetlbfs: ERROR: " __VA_ARGS__)
+ do { \
+ if (__hugetlbfs_verbose >= 1) { \
+ fprintf(stderr, "libhugetlbfs: ERROR: " __VA_ARGS__); \
+ fprintf(stderr, "\n"); \
+ } \
+ } while (0)
#define WARNING(...) \
- if (__hugetlbfs_verbose >= 2) \
- fprintf(stderr, "libhugetlbfs: WARNING: " __VA_ARGS__)
+ do { \
+ if (__hugetlbfs_verbose >= 2) { \
+ fprintf(stderr, "libhugetlbfs: WARNING: " __VA_ARGS__);
\
+ fprintf(stderr, "\n"); \
+ } \
+ } while (0)
#define DEBUG(...) \
- if (__hugetlbfs_verbose >= 3) \
- fprintf(stderr, "libhugetlbfs: " __VA_ARGS__)
+ do { \
+ if (__hugetlbfs_verbose >= 3) { \
+ fprintf(stderr, "libhugetlbfs: " __VA_ARGS__); \
+ fprintf(stderr, "\n"); \
+ } \
+ } while (0)
#define DEBUG_CONT(...) \
- if (__hugetlbfs_verbose >= 3) \
- fprintf(stderr, __VA_ARGS__)
+ do { \
+ if (__hugetlbfs_verbose >= 3) { \
+ fprintf(stderr, __VA_ARGS__); \
+ fprintf(stderr, "\n"); \
+ } \
+ } while (0)
#if defined(__powerpc64__) && !defined(__LP64__)
/* Older binutils fail to provide this symbol */
diff --git a/morecore.c b/morecore.c
index 5fdcb24..7fe3ae1 100644
--- a/morecore.c
+++ b/morecore.c
@@ -55,7 +55,7 @@ static void *hugetlbfs_morecore(ptrdiff_t increment)
void *p;
long newsize = 0;
- DEBUG("hugetlbfs_morecore(%ld) = ...\n", (long)increment);
+ DEBUG("hugetlbfs_morecore(%ld) = ...", (long)increment);
/*
* how much to grow the heap by =
@@ -63,7 +63,7 @@ static void *hugetlbfs_morecore(ptrdiff_t increment)
*/
newsize = (heaptop-heapbase) + increment - mapsize;
- DEBUG("heapbase = %p, heaptop = %p, mapsize = %lx, newsize=%ld\n",
+ DEBUG("heapbase = %p, heaptop = %p, mapsize = %lx, newsize=%ld",
heapbase, heaptop, mapsize, newsize);
/* growing the heap */
@@ -74,27 +74,27 @@ static void *hugetlbfs_morecore(ptrdiff_t increment)
*/
newsize = ALIGN(newsize, blocksize);
- DEBUG("Attempting to map %ld bytes\n", newsize);
+ DEBUG("Attempting to map %ld bytes", newsize);
/* map in (extend) more of the file at the end of our last map
*/
p = mmap(heapbase + mapsize, newsize, PROT_READ|PROT_WRITE,
MAP_PRIVATE, heap_fd, mapsize);
if (p == MAP_FAILED) {
- WARNING("Mapping failed in hugetlbfs_morecore()\n");
+ WARNING("Mapping failed in hugetlbfs_morecore()");
return NULL;
}
/* if this is the first map */
if (! mapsize) {
if (heapbase && (heapbase != p))
- WARNING("Heap originates at %p instead of %p\n",
+ WARNING("Heap originates at %p instead of %p",
p, heapbase);
/* then setup the heap variables */
heapbase = heaptop = p;
} else if (p != (heapbase + mapsize)) {
/* Couldn't get the mapping where we wanted */
munmap(p, newsize);
- WARNING("Mapped at %p instead of %p in
hugetlbfs_morecore()\n",
+ WARNING("Mapped at %p instead of %p in
hugetlbfs_morecore()",
p, heapbase + mapsize);
return NULL;
}
@@ -107,7 +107,7 @@ static void *hugetlbfs_morecore(ptrdiff_t increment)
/* Use mlock to guarantee these pages to the process */
ret = mlock(p, newsize);
if (ret) {
- WARNING("Failed to reserve huge pages in
hugetlbfs_morecore()\n");
+ WARNING("Failed to reserve huge pages in
hugetlbfs_morecore()");
munmap(p, newsize);
return NULL;
}
@@ -123,7 +123,7 @@ static void *hugetlbfs_morecore(ptrdiff_t increment)
/* and we now have added this much more space to the heap */
heaptop = heaptop + increment;
- DEBUG("... = %p\n", p);
+ DEBUG("... = %p", p);
return p;
}
@@ -136,20 +136,20 @@ static void __attribute__((constructor))
setup_morecore(void)
if (! env)
return;
if (strcasecmp(env, "no") == 0) {
- DEBUG("HUGETLB_MORECORE=%s, not setting up morecore\n",
+ DEBUG("HUGETLB_MORECORE=%s, not setting up morecore",
env);
return;
}
blocksize = gethugepagesize();
if (! blocksize) {
- ERROR("Hugepages unavailable\n");
+ ERROR("Hugepages unavailable");
return;
}
heap_fd = hugetlbfs_unlinked_fd();
if (heap_fd < 0) {
- ERROR("Couldn't open hugetlbfs file for morecore\n");
+ ERROR("Couldn't open hugetlbfs file for morecore");
return;
}
@@ -157,7 +157,7 @@ static void __attribute__((constructor))
setup_morecore(void)
if (env) {
heapaddr = strtoul(env, &ep, 16);
if (*ep != '\0') {
- ERROR("Can't parse HUGETLB_MORECORE_HEAPBASE: %s\n",
+ ERROR("Can't parse HUGETLB_MORECORE_HEAPBASE: %s",
env);
return;
}
@@ -166,7 +166,7 @@ static void __attribute__((constructor))
setup_morecore(void)
heapaddr = ALIGN(heapaddr, hugetlbfs_vaddr_granularity());
}
- DEBUG("setup_morecore(): heapaddr = 0x%lx\n", heapaddr);
+ DEBUG("setup_morecore(): heapaddr = 0x%lx", heapaddr);
heaptop = heapbase = (void *)heapaddr;
__morecore = &hugetlbfs_morecore;
--
Nishanth Aravamudan <[EMAIL PROTECTED]>
IBM Linux Technology Center
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Libhugetlbfs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/libhugetlbfs-devel
