Some quick notes inline. On 05/01/2011 04:22 PM, Lukas Huba wrote: > > ------------------------------------------------------------------------------ > (add/remove: 18/0 grow/shrink: 4/0 up/down: 2875/0) Total: 2875 > bytes > text data bss dec hex filename > 29947 1674 8320 39941 9c05 busybox_old > 34547 1806 8336 44689 ae91 busybox_unstripped
I'd say the .bss growth is needlessly too big. > include/applets.src.h | 1 + > include/usage.src.h | 8 ++ > networking/Config.src | 16 ++++ > networking/Kbuild.src | 1 + > networking/portmap.c | 213 > +++++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 239 insertions(+), 0 deletions(-) > create mode 100644 networking/portmap.c > It is no longer needed to modify those files. You should put all these information into the portmap.c file, see for instance the util-linux/rev.c. > diff --git a/networking/portmap.c b/networking/portmap.c > new file mode 100644 > index 0000000..6dc2f5e > --- /dev/null > +++ b/networking/portmap.c > @@ -0,0 +1,213 @@ > +/* vi: set sw=4 ts=4: */ > +/* > + * mini portmap implementation for busybox > + * > + * Copyright (C) 2011 by Lukas Huba <[email protected]> > + * > + * Licensed under GPLv2, see file LICENSE in this source tree. > + */ > + > +#include "libbb.h" > +#include <rpc/rpc.h> > +#include <rpc/pmap_prot.h> > +#include <net/if.h> > +#include <sys/ioctl.h> > +#include <syslog.h> > + > +/* commandline parameters */ > +int flags = 0; > +#define FLAGS_ARG "h:pr" > +#define FLAG_HOST (1 << 0) > +#define FLAG_SPORT (2 << 0) > +#define FLAG_SREMOTE (4 << 0) > + > +/* portmap items */ > +int pn = 0; > +struct pmaplist *pl = NULL; > + > +/* default count of network interfaces (later it updates itself) > + * it's for higher efficiency (re|m)alloc */ > +int ifs = 5; These globals at least should be static. Also, no need to assign `0' to global variable since these are zeroed automatically. However, you definitely should use the struct globals trick to reduce the .bss usage. > +static bool_t _pmapproc_set(struct pmap *pr) { > Missing newline before the curly bracket. ^^ Also, `const struct' may be used here. > + struct pmaplist *p, *pp = NULL, **n; > + if (pn >= CONFIG_PORTMAP_ITEMS_MAX) { > + bb_error_msg("All available resources are used! Count of > registered "\ > + "RPC services is limited to %i.", > CONFIG_PORTMAP_ITEMS_MAX); > + return FALSE; > + } > + for(p=pl; p!=NULL; p=p->pml_next) { Missing spaces before ( and around !=/=. > + if (p->pml_map.pm_prog == pr->pm_prog && > + p->pml_map.pm_vers == pr->pm_vers && > + p->pml_map.pm_prot == pr->pm_prot) { Move the `&&'s to the start of the line. > + return FALSE; > + } > + pp=p; Spaces. > + } > + if (pp == NULL) { > + n = &pl; > + } else { > + n = &(pp->pml_next); Redundant braces. > + } > + (*n) = (struct pmaplist *)xmalloc(sizeof(struct pmaplist)); Redundant cast. > + (*n)->pml_map = *pr; > + (*n)->pml_next = NULL; > + pn++; > + return TRUE; > +} > + > +static bool_t _pmapproc_unset(struct pmap *pr) { [ ... ] Spaces. Also, do we really want functions like these to start with an underscore? > +static bool_t check_security(SVCXPRT *xprt) { > + if (!(flags & FLAG_SPORT)) { > + /* secure mode > + * SET and UNSET procs can be exec only from port <= 1024 */ > + if (htons(svc_getcaller(xprt)->sin_port) > 1024) return FALSE; Don't put the `return' statement to the same line where `if' is. > + } > + if (!(flags & FLAG_SREMOTE)) { > + /* secure mode > + * SET and UNSET procs can be exec only from local machine */ > + int s, cnt = 0; Why is cnt zeroed? > + struct ifconf ifc; > + s = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP); I believe here should be used `xsocket'. > + ifc.ifc_req = NULL; I don't see why the assignment here is needed. > + for(cnt=0; !cnt||ifc.ifc_len/sizeof(struct ifreq)==cnt; > cnt+=ifs) { Spaces. > + ifc.ifc_len = (cnt+ifs)*sizeof(struct ifreq); > + ifc.ifc_req = (struct ifreq *)xrealloc(ifc.ifc_req, > ifc.ifc_len); Redundant cast. > + if (ioctl(s, SIOCGIFCONF, &ifc) < 0) { > + bb_error_msg("ioctl(SIOCGIFCONF) error"); Trailing whitespace at the end of the line. > + free(ifc.ifc_req); > + return FALSE; > + } > + } > + ifs = ifc.ifc_len/sizeof(struct ifreq)+1; > + for(int i=0; i<cnt; i++) { > + if (((struct sockaddr_in *)&ifc.ifc_req[i].ifr_addr) > + ->sin_addr.s_addr == > svc_getcaller(xprt)->sin_addr.s_addr) { A line starting with `->' is kinda confusing. > + free(ifc.ifc_req); > + return TRUE; > + } > + } > + free(ifc.ifc_req); > + return FALSE; I would use something like this instead of repeating free+return: if (...) goto out; out: free(ifc.ifc_req); return FALSE; > + bb_error_msg_and_die("Cannot start portmap!"); This message should not start with a capital. Also, remove the `!'. Marek _______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
