On Tue, Oct 18, 2011 at 9:41 AM, Leonid Lisovskiy <[email protected]> wrote:
>> const char reason[] = "REASON=NBI";
>> ...
>> *curr = xstrdup(reason);
>>
>> Why reason[] exists?
>
> Historical only, but exists in most manuals. For example -
> http://manpages.ubuntu.com/manpages/hardy/man8/dhcp6c.8.html
I meant, why it exists as an array? Just xstrdup("REASON=NBI")...
>> Elsewhere, random observations:
>>
>> memset(&hints, 0, sizeof(hints));
>> hints.ai_family = PF_INET6;
>> hints.ai_socktype = SOCK_DGRAM;
>> hints.ai_protocol = IPPROTO_UDP;
>> error = getaddrinfo(DH6ADDR_ALLAGENT, DH6PORT_UPSTREAM, &hints, &res);
>> if (error) {
>> bb_error_msg_and_die("getaddrinfo: %s", gai_strerror(error));
>> }
>> memcpy(&G.sa6_allagent, res->ai_addr, res->ai_addrlen);
>> freeaddrinfo(res);
>>
>> ???! This is a fixed IPv6 address, [ff02::1:2]:547, it can be just a
>> constant struct,
>> right?
>
> This code come unmodified from wide-dhcpv6, seems to be you are right,
> I should do this optimization.
And more generally, isn't xdotted2sockaddr("host[:port]") better
instead of this multi-line horror?
>> argv += optind;
>> argc -= optind;
>> if (argc < 1) {
>> bb_show_usage();
>> }
>>
>> if (!(opt & OPT_FOREGROUND) && !(opt & OPT_i)) {
>> bb_daemonize_or_rexec(DAEMON_CLOSE_EXTRA_FDS, argv);
>>
>> BUG: would re-execute (on NOMMU) with wrong argv: argv += optind
>> is done too early.
>
> Sorry, I haven't NOMMU for tests. I should move argv/argc
> modifications after bb_daemonize_or_rexec(), right?
Yes. And you do not need to modify argc. The only usage is
+ if (argc < 1) {
+ bb_show_usage();
and it can be expressed with an opt_complementary rule "min 1 arg needed".
>> Need documentation for /etc/dhcp6c.conf format.
>
> i.e. self-commented samples/udhcp/dhcp6c.conf isn't enough?
Oops, I missed it... yes, it's enough.
>> Patch does not compile with uclibc:
>>
>> networking/udhcp/lib.a(if6.o): In function `gethwid':
>> if6.c:(.text.gethwid+0xf): undefined reference to `getifaddrs'
>> if6.c:(.text.gethwid+0x9a): undefined reference to `freeifaddrs'
>> networking/udhcp/lib.a(if6.o): In function `if6init':
>> if6.c:(.text.if6init+0x88): undefined reference to `getifaddrs'
>> if6.c:(.text.if6init+0x10d): undefined reference to `freeifaddrs'
>
> Which uClibc version you are use? Have UCLIBC_SUPPORT_AI_ADDRCONFIG
> option turned on?
It's uclibc-0.9.32-git. Configured with
!UCLIBC_USE_NETLINK, therefore !UCLIBC_SUPPORT_AI_ADDRCONFIG.
> Are use of getifaddrs/freeifaddrs prohibited in bb?
I guess it's not a very good idea to use them, they are not portable.
Why dhch6 even needs to know some intimate things about interfaces?
I only understand the need to know MAC, nothing else.
BTW, udhcpc is handling just one iface, which simplifies logic a lot.
What do you think about using the same approach for DHCPv6?
>> networking/udhcp/lib.a(common6.o): In function `duidstr':
>> common6.c:(.text.duidstr+0x9): undefined reference to
>> `BUG_dhcp6c_globals_too_big'
>> collect2: ld returned 1 exit status
>> make: *** [busybox_unstripped] Error 1
>
> Can I ask you about COMMON_BUFSIZE value on your system?
1k, I guess. That's the guaranteed minimum.
More funky fragments:
+static ALWAYS_INLINE void hmacmd5_invalidate(hmacmd5_t *ctx)
+{
+ memset(ctx, 0, sizeof(ctx));
+}
(1) Shouldn't it be sizeof(*ctx)?
(2) Why do you need this function if there is only one callsite?
+static ALWAYS_INLINE void hmacmd5_update(hmacmd5_t *ctx,
+ const unsigned char *buf, unsigned int len)
+{
+ md5_hash(&ctx->md5ctx, buf, len);
+}
I'd just use md5_hash(&ctx->md5ctx, buf, len) directly, w/o this wrapper.
+ if (time(&now) == -1)
+ return -1; /* treat it as expiration (XXX) */
In busybox, we tend to think that time() never fails.
dp->d1.time = htonl((uint32_t)(t64 & 0xffffffffUL));
"& 0xffffffffUL" is superfluous.
memcpy((void *)dp + len, tmpbuf, hwlen);
Adding to (void*) ptr is gcc-ism. Use (char*).
+ if ((fd = open(idfile, O_WRONLY)) < 0) {
Please move assignments out of if (expr).
+ bb_error_msg("failed to write DUID file");
Shorter string please: "can't write ...."
+ log1("send %s to %s", dhcp6msgstr(dh6->dh6_msgtype),
+ xmalloc_sockaddr2dotted_noport((struct sockaddr *)&dst));
This leaks malloced ptr.
+ /* find the corresponding event based on the received xid */
+ ev = find_event_withid(ifp->ifid, ntohl(dh6->dh6_xid) & DH6_XIDMASK);
+ if (ev == NULL) {
+ bb_error_msg("XID mismatch");
+ goto fail;
+ }
Why we emit a message? It's just a packet to somebody else.
I think it needs to be log1()...
+ dst->dv_buf = malloc_or_warn(dst->dv_len);
+ if (dst->dv_buf == NULL)
+ return -1;
Why not xmalloc? xmalloc will make dhcp6_vbuf_copy()
always succeed, which in turn will make dhcp6_copy_options()
always succeed, which in turn will make this error checking
unnecessary:
+ if (dhcp6_copy_options(&newserver->optinfo, optinfo)) {
+ bb_error_msg("failed to copy options");
+ free(newserver->authparam);
+ free(newserver);
+ return -1;
+ }
Think about it. What a hell does it mean "failed to copy options"?
CPU lost the ability to copy data? - That doesn't make sense.
Not enough memory? - Busybox always handled that by simply dying.
+ log1("got an expected reply, sleeping.");
Remove trailing period. It's bloat.
+static ssize_t getifhwaddr(const char *ifname, char *buf, uint16_t *hwtypep,
+ int ppa)
+{
+ int fd, flags;
+ char fname[MAXPATHLEN], *cp;
+ struct strbuf putctl;
+ struct strbuf getctl;
+ long getbuf[1024];
+ dl_info_req_t dlir;
+ dl_phys_addr_req_t dlpar;
+ dl_phys_addr_ack_t *dlpaa;
+
+ log1("trying %s ppa %d", ifname, ppa);
+
+ if (ifname[0] == '\0')
+ return -1;
+ if (ppa >= 0 && !isdigit(ifname[strlen(ifname) - 1]))
+ snprintf(fname, sizeof(fname), "/dev/%s%d", ifname, ppa);
+ else
+ snprintf(fname, sizeof(fname), "/dev/%s", ifname);
I think MAXPATHLEN is often 4k or 8k. Wasting that many bytes to store
/dev/<iface_name><number> is wrong.
+ getctl.maxlen = sizeof(getbuf);
+ getctl.buf = (char *)getbuf;
+ if ((fd = open(fname, O_RDWR)) == -1) {
+ }
+ dlir.dl_primitive = DL_INFO_REQ;
+ putctl.len = sizeof(dlir);
+ putctl.buf = (char *)&dlir;
+ if (putmsg(fd, &putctl, NULL, 0) == -1) {
+ ...
+ close(fd);
+ return -1;
+ }
What is going on here? Linux doesn't have /dev/eth0.
Neither it has putmsg(). Ah, it is in #ifdef __sun__.
We probably don't need this part.
+ unsigned long long now = monotonic_ms();
...
+ if (tm->t <= now) {
It's not rollover-safe way to compare timestampts.
Use if ((long long)(now - tm->t) >= 0) ...
+ if (t_diff >= (MAX_ELAPSED_TIME + 100LL) * 10LL) {
Why LL?
--
vda
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
