[PATCH] portmap: new applet

[Marek Polacek]

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 <huba.lukas at centrum.cz>
> + *
> + * 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