Hi, thanks for your review.
2010/3/26 Denys Vlasenko <[email protected]>:
> 2010/3/24 Jérémie Koenig <[email protected]>:
>> (...)
>> 0c3df06 kconfig: fix #include ordering in the bison grammar
>> 40293cf kconfig: drop PATH_MAX
>
> I think Hurd is much better off introducing PATH_MAX and stop
> trying to steer all Unix world their way. That won't work.
I guess it's a matter of philosophy on their end, since the whole
point of Hurd is to remove arbitrary limits whenever possible. On the
other hand, I understand your point as well (after all, standards
exist for a reason and following them to the letter to the detriment
of that purpose is dubious).
>> (...)
>> 1b81c60 start-stop-daemon: drop PATH_MAX
>
> + ssize_t bytes = MAX(strlen("/proc//cmdline") + sizeof(int)*3,
> + strlen(execname) + 1);
> + char *buf = alloca(bytes + 1);
>
> (1) You should use sizeof() on constant string, not strlen.
> (2) strlen(execname) on every check? I don't like it.
>
>> fb34704 libbb: use sysconf() to query the page size
>
> What was wrong with defaulting to 4k?
The problem is more with locating the PAGE_SIZE constant: Linux
defines it as (sysconf(_SC_PAGESIZE)) in <sys/user.h>, and Hurd
defines it to a constant value in <mach/vm_param.h>. Apparently
neither of these headers is specified by POSIX and neither Linux nor
Hurd define PAGE_SIZE in <limits.h> as (optionally) suggested by
POSIX. On the other hand, sysconf(_SC_PAGESIZE) is mandated by POSIX
to be available from <unistd.h> and works at least on Linux and Hurd
(it seems defined on FreeBSD as well).
Now of course, if using PAGE_SIZE is preferable for whatever reason I
have no problem with your approach, but we need a special case for
Hurd to include the right header. (see the attached patch)
>> e8f57a6 libbb: utsname.dommainname is linux-only
>
> So, how Hurd reports domain name of the machine?
Through getdomainname(), which is not specified by POSIX either, but
seems more widespread. (see the attached patch -- I'm not sure whether
the 'size > 0' safeguard is desirable or just increases code size and
obscureness for no good reason)
>> 88782c4 libbb: setsockopt_bindtodevice() portability stub
>> 5a913d8 umount: use xmalloc_realpath() and drop PATH_MAX
>
> GETMNTENT_BUFSIZE of "only" 16k? 80x25 screen is 2k.
> 16k is 8 screenfuls of text, filled edge to edge.
> Do you really think this is such a pressing issue
> to support mnt entries THAT big?
I have to admit that I was somewhat disturbed by the fact that
getmntent_r() would silently truncate the record if the buffer was too
small. This is also why I posted the patch event after I realized that
umount would never compile on Hurd, which does not have a umount()
call at all. (Sorry, I should have been more explicit about this.)
Thinking about it, one could initialize a small buffer to some
non-zero value, and check whether the last byte has been overwritten
by getmntent_r(). In this case, we could double the buffer size and
restart parsing the file. Let me know if you would like me to give it
a try.
> I prefer instead:
>
> #ifndef PATH_MAX
> # define PATH_MAX (4*1024)
> #endif
Indeed, this is less complicated and more reasonable, though as
mentioned above I'm not sure it would ever be needed (sorry again for
the confusion).
(...)
>> (the patches are attached)
> Applied with some changes. Please try current git.
Except for <sys/user.h> not being available, it works great!
Thanks,
--
Jérémie Koenig <[email protected]>
http://jk.fr.eu.org/
From a626b97420cc121a21190a33066119059fac03a0 Mon Sep 17 00:00:00 2001
From: Jeremie Koenig <[email protected]>
Date: Mon, 29 Mar 2010 07:06:24 +0200
Subject: [PATCH 1/2] libbb: use getdomainname() on non-Linux systems
---
libbb/safe_gethostname.c | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/libbb/safe_gethostname.c b/libbb/safe_gethostname.c
index e93254b..7a1aae7 100644
--- a/libbb/safe_gethostname.c
+++ b/libbb/safe_gethostname.c
@@ -65,6 +65,22 @@ char* FAST_FUNC safe_getdomainname(void)
uname(&uts);
return xstrndup(!uts.domainname[0] ? "?" : uts.domainname, sizeof(uts.domainname));
#else
+ int size = 1;
+ char *buf = NULL;
+ int r;
+
+ do {
+ buf = xrealloc(buf, size);
+ buf[size-1] = '\0';
+
+ r = getdomainname(buf, size);
+ if (r >= 0 && buf[size-1] == '\0')
+ return buf;
+
+ size *= 2;
+ } while ((r >= 0 || errno == EINVAL) && size > 0);
+
+ free(buf);
return xstrdup("?");
#endif
}
--
1.7.0.2
From 6959546c25927094f4dcde325f5458011a81577e Mon Sep 17 00:00:00 2001
From: Jeremie Koenig <[email protected]>
Date: Mon, 29 Mar 2010 07:31:50 +0200
Subject: [PATCH 2/2] libbb: locate PAGE_SIZE on Hurd
---
libbb/appletlib.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/libbb/appletlib.c b/libbb/appletlib.c
index b31532a..4c50ee6 100644
--- a/libbb/appletlib.c
+++ b/libbb/appletlib.c
@@ -30,7 +30,13 @@
#include "busybox.h"
#include <assert.h>
#include <malloc.h>
-#include <sys/user.h> /* PAGE_SIZE */
+
+/* PAGE_SIZE */
+#ifdef __GNU__
+#include <mach/vm_param.h>
+#else
+#include <sys/user.h>
+#endif
/* Declare <applet>_main() */
--
1.7.0.2
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
