dhcpcd-discuss

-<| Quoting Roy Marples <roy@xxxxxxxxxxxx>, on Tuesday, 2017-01-24 17:42:49 |>-
> Comments on the patch.

Thank you! I’ve prepared a v3 patch including documentation and
some modifications.

> On 2017-01-24 15:43, Philipp Gesang wrote:
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -760,7 +760,7 @@ make_message(struct bootp **bootpm, const struct
> > interface *ifp, uint8_t type)
> >  	if ((mtu = if_getmtu(ifp)) == -1)
> >  		logger(ifp->ctx, LOG_ERR,
> >  		    "%s: if_getmtu: %m", ifp->name);
> > -	else if (mtu < MTU_MIN) {
> > +	else if (mtu < MTU_MIN && ifo->options & DHCPCD_CONFIGURE_LINK) {
> >  		if (if_setmtu(ifp, MTU_MIN) == -1)
> >  			logger(ifp->ctx, LOG_ERR,
> >  			    "%s: if_setmtu: %m", ifp->name);
> 
> So if interface MTU < MTU_MIN we can't actually send or recieve a DHCP
> message.
> Is this the behaviour you want?

Not necessarily. The goal is to not modify the link so the patch
works for us (link MTU is handled elsewhere). Of course this may
not be satisfactory for everyone. What would you suggest? Error
out because dhcpcd can’t operate sanely?

> > diff --git a/if-linux.c b/if-linux.c
> > index 26f4d3c..1957c6a 100644
> > --- a/if-linux.c
> > +++ b/if-linux.c
> > @@ -226,6 +226,12 @@ if_init(struct interface *ifp)
> >  		return errno == ENOENT ? 0 : -1;
> >  	if (n == 1)
> >  		return 0;
> > +	if (!(ifp->ctx->options & DHCPCD_CONFIGURE_LINK)) {
> > +		logger(ifp->ctx, LOG_DEBUG,
> > +		       "%s: leaving IPv4 sysctl promote_secondaries unset",
> > +		       ifp->name);
> > +		return 0;
> > +	}
> >  	return write_path(path, "1") == -1 ? -1 : 0;
> >  }
> > 
> > diff --git a/if-options.c b/if-options.c
> > index 7dd6a43..02b69db 100644
> > --- a/if-options.c
> > +++ b/if-options.c
> > @@ -104,6 +104,7 @@
> >  #define O_LASTLEASE_EXTEND	O_BASE + 46
> >  #define O_INACTIVE		O_BASE + 47
> >  #define	O_MUDURL		O_BASE + 48
> > +#define	O_NOCONFIGURE		O_BASE + 49
> > 
> >  const struct option cf_options[] = {
> >  	{"background",      no_argument,       NULL, 'b'},
> > @@ -205,6 +206,7 @@ const struct option cf_options[] = {
> >  	{"lastleaseextend", no_argument,       NULL, O_LASTLEASE_EXTEND},
> >  	{"inactive",        no_argument,       NULL, O_INACTIVE},
> >  	{"mudurl",          required_argument, NULL, O_MUDURL},
> > +	{"noconfigure",     no_argument,       NULL, O_NOCONFIGURE},
> >  	{NULL,              0,                 NULL, '\0'}
> >  };
> > 
> > @@ -2146,6 +2148,10 @@ err_sla:
> >  		}
> >  		*ifo->mudurl = (uint8_t)s;
> >  		break;
> > +	case O_NOCONFIGURE:
> > +		logger(ctx, LOG_DEBUG, "all: suppressing link configuration");
> 
> "all" isn't strictly true.
> This could be a per interface option and ifname is passed to the function.
>  +		logger(ctx, LOG_DEBUG, "%s: suppressing link configuration", ifname);
> 
> > +		ifo->options &= ~DHCPCD_CONFIGURE_LINK;
> > +		break;
> >  	default:
> >  		return 0;
> >  	}
> > @@ -2251,7 +2257,8 @@ default_config(struct dhcpcd_ctx *ctx)
> >  		logger(ctx, LOG_ERR, "%s: %m", __func__);
> >  		return NULL;
> >  	}
> > -	ifo->options |= DHCPCD_IF_UP | DHCPCD_LINK | DHCPCD_INITIAL_DELAY;
> > +	ifo->options |= DHCPCD_IF_UP | DHCPCD_LINK | DHCPCD_INITIAL_DELAY
> > +		| DHCPCD_CONFIGURE_LINK;
> >  	ifo->timeout = DEFAULT_TIMEOUT;
> >  	ifo->reboot = DEFAULT_REBOOT;
> >  	ifo->metric = -1;
> > diff --git a/if-options.h b/if-options.h
> > index 3f2eb04..93216f6 100644
> > --- a/if-options.h
> > +++ b/if-options.h
> > @@ -118,6 +118,7 @@
> >  #define DHCPCD_PRINT_PIDFILE		(1ULL << 59)
> >  #define DHCPCD_ONESHOT			(1ULL << 60)
> >  #define DHCPCD_INACTIVE			(1ULL << 61)
> > +#define DHCPCD_CONFIGURE_LINK		(1ULL << 62)
> > 
> >  #define DHCPCD_NODROP	(DHCPCD_EXITING | DHCPCD_PERSISTENT)
> > 
> > diff --git a/ipv4.c b/ipv4.c
> > index 57a19df..33efcea 100644
> > --- a/ipv4.c
> > +++ b/ipv4.c
> > @@ -513,6 +513,7 @@ delete_address(struct interface *ifp)
> >  	 * by a 3rd party. */
> >  	if (state->addr == NULL ||
> >  	    ifo->options & DHCPCD_INFORM ||
> > +	    !(ifo->options & DHCPCD_CONFIGURE_LINK) ||
> >  	    (ifo->options & DHCPCD_STATIC && ifo->req_addr.s_addr == 0))
> >  		return 0;
> >  	r = ipv4_deladdr(state->addr, 0);
> > @@ -638,15 +639,20 @@ ipv4_addaddr(struct interface *ifp, const struct
> > in_addr *addr,
> >  		    ia->alias, ia->saddr);
> >  #endif
> > 
> > -	logger(ifp->ctx, LOG_DEBUG, "%s: adding IP address %s broadcast %s",
> > -	    ifp->name, ia->saddr, inet_ntoa(*bcast));
> > -	if (if_address(RTM_NEWADDR, ia) == -1) {
> > -		if (errno != EEXIST)
> > -			logger(ifp->ctx, LOG_ERR, "%s: if_addaddress: %m",
> > -			    __func__);
> > -		free(ia);
> > -		return NULL;
> > -	}
> > +	if (ifp->options->options & DHCPCD_CONFIGURE_LINK) {
> > +		logger(ifp->ctx, LOG_DEBUG, "%s: adding IP address %s broadcast %s",
> > +		       ifp->name, ia->saddr, inet_ntoa(*bcast));
> > +		if (if_address(RTM_NEWADDR, ia) == -1) {
> > +			if (errno != EEXIST)
> > +				logger(ifp->ctx, LOG_ERR, "%s: if_addaddress: %m",
> > +				       __func__);
> > +			free(ia);
> > +			return NULL;
> > +		}
> > +	} else
> > +		logger(ifp->ctx, LOG_DEBUG, "%s: skipping link config with lease "
> > +		       "data IP address %s broadcast %s",
> > +		       ifp->name, ia->saddr, inet_ntoa(*bcast));
> > 
> >  #ifdef ALIAS_ADDR
> >  	if (replaced) {
> > diff --git a/route.c b/route.c
> > index 1e6bd84..3da6cde 100644
> > --- a/route.c
> > +++ b/route.c
> > @@ -290,6 +290,8 @@ rt_add(struct rt *nrt, struct rt *ort)
> >  	    sa_is_unspecified(&nrt->rt_dest) &&
> >  	    sa_is_unspecified(&nrt->rt_netmask))
> >  		return false;
> > +	if (!(options & DHCPCD_CONFIGURE_LINK))
> > +		return false;
> > 
> >  	rt_desc(ort == NULL ? "adding" : "changing", nrt);
> > 
> > @@ -394,6 +396,8 @@ rt_delete(struct rt *rt)
> >  {
> >  	int retval;
> > 
> > +	if (!(rt->rt_ifp->options->options &  DHCPCD_CONFIGURE_LINK))
> > +		return false;
> >  	rt_desc("deleting", rt);
> >  	retval = if_route(RTM_DELETE, rt) == -1 ? false : true;
> >  	if (!retval && errno != ENOENT && errno != ESRCH)
> 
> Otherwise, looks ok.

Thanks for the review. It’d be great if we could have this
functionality for 7.x.

Best,
Philipp

Attachment: signature.asc
Description: PGP signature