On Mon, Nov 17, 2008 at 12:48:58PM -0600, Adam Litke wrote:
> On Mon, 2008-11-17 at 17:23 +0000, Mel Gorman wrote:
> > > @@ -276,7 +276,7 @@ static int find_or_create_share_path(long page_size)
> > >
> > > ret = mkdir(share_readonly_path, 0700);
> > > if ((ret != 0) && (errno != EEXIST)) {
> > > - ERROR("Error creating share directory %s\n",
> > > + WARNING("Error creating share directory %s\n",
> > > share_readonly_path);
> > > return -1;
> > > }
> >
> > hmm, I'm 50/50 on this one. It implies to me that sharing is disabled so
> > if this is now a warning, can we tell the user that sharing is disabled in
> > it? I think if I saw "ERROR", I would automatically assume it was busted
> > but not so sure about a warning.
>
> My working definition of WARNING was that an event occurred that we can
> handle, but with altered semantics. In this case, we were unable to
> create the share directory but we can continue, albeit with sharing
> disabled.
>
Ok, that seems sensible. Change it to WARNING but add a note saying
we're falling back. This might be something we want to do elsewhere too
so that it's clear semantics are changing from what is expected. Would
that make sense?
> >
> > > @@ -284,24 +284,24 @@ static int find_or_create_share_path(long page_size)
> > > /* Check the share directory is sane */
> > > ret = lstat(share_readonly_path, &sb);
> > > if (ret != 0) {
> > > - ERROR("Couldn't stat() %s: %s\n", share_readonly_path,
> > > + WARNING("Couldn't stat() %s: %s\n", share_readonly_path,
> > > strerror(errno));
> > > return -1;
> > > }
> > >
> > > if (! S_ISDIR(sb.st_mode)) {
> > > - ERROR("%s is not a directory\n", share_readonly_path);
> > > + WARNING("%s is not a directory\n", share_readonly_path);
> > > return -1;
> > > }
> > >
> > > if (sb.st_uid != getuid()) {
> > > - ERROR("%s has wrong owner (uid=%d instead of %d)\n",
> > > + WARNING("%s has wrong owner (uid=%d instead of %d)\n",
> > > share_readonly_path, sb.st_uid, getuid());
> > > return -1;
> > > }
> > >
> > > if (sb.st_mode & (S_IWGRP | S_IWOTH)) {
> > > - ERROR("%s has bad permissions 0%03o\n",
> > > + WARNING("%s has bad permissions 0%03o\n",
> > > share_readonly_path, sb.st_mode);
> > > return -1;
> > > }
> >
> > Similarly, all these look like errors to me.
>
> They all result in us disabling sharing and continuing with un-shared
> remapping.
>
> > > @@ -319,7 +319,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);
> > > + DEBUG("Non-zero BSS data @ %p: %lx\n", addr, *addr);
> > > }
> > > }
> >
> > What changed that this is now DEBUG instead of WARNING?
>
> It should have always been DEBUG. We never run this function unless
> debugging is enabled.
>
Ok.
> >
> > >
> > > @@ -347,14 +347,14 @@ static int get_shared_file_name(struct seg_info
> > > *htlb_seg_info, char *file_path)
> > > memset(binary, 0, sizeof(binary));
> > > ret = readlink("/proc/self/exe", binary, PATH_MAX);
> > > if (ret < 0) {
> > > - ERROR("shared_file: readlink() on /proc/self/exe "
> > > + WARNING("shared_file: readlink() on /proc/self/exe "
> > > "failed: %s\n", strerror(errno));
> > > return -1;
> > > }
> > >
> > > binary2 = basename(binary);
> > > if (!binary2) {
> > > - ERROR("shared_file: basename() on %s failed: %s\n",
> > > + WARNING("shared_file: basename() on %s failed: %s\n",
> > > binary, strerror(errno));
> > > return -1;
> > > }
> >
> > They all look like errors to me that result in broken sharing.
>
> We seem to disagree on the definition of a warning. Since all of these
> cases result in fallback to no sharing, I'd consider them noteworthy
> warning messages.
>
I'm ok with that definition as long as it's clear to the user the
semantics change from what is expected. I don't think that is something
we currently do.
> <snip>
>
> > > @@ -1063,7 +1064,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");
> > > + WARNING("Falling back to unlinked files\n");
> >
> > INFO? I think the other messages that would lead to this are INFO at the
> > moment.
>
> I think it's a mixture. Here we are indicating that we plan to continue
> with altered semantics. That makes it a warning to me.
>
> <snip>
>
> --
> Adam Litke - (agl at us.ibm.com)
> IBM Linux Technology Center
>
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
Libhugetlbfs-devel mailing list
Libhugetlbfs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libhugetlbfs-devel
