Re: dhclient support for /32 assignments
On Wed, Dec 04, 2013 at 12:47:19PM -0800, Matthew Dempsky wrote: On Wed, Dec 04, 2013 at 02:10:21PM -0500, Kenneth R Westerback wrote: No, that was my point. i.e. don't avoid adding the route when given a /32 address just because class static routes are also present. I think there might be a misunderstanding, so let me back up and try to clarify. :) Compute Engine gives out DHCP leases like ip: 10.240.77.88, netmask: 255.255.255.255, gateway: 10.240.0.1. The problem is 10.240.0.1 isn't routable given a 10.240.77.88/32 IP address. Ah. That was *not* what I understood you were saying, so there was indeed a misunderstanding. :-) ISC DHCP on Linux handles this by special casing netmask == 255.255.255.255, and adding an extra direct route for the gateway IP. E.g., for the above assignment, ISC DHCP would run route add 10.240.0.1 dev eth0 which tells Linux's network stack 10.240.0.1 is directly accessible via eth0, even though it's not part of the 10.240.77.88/32 subnet. (It would then run route add default 10.240.0.1 as per normal.) This seems weird and wrong to me, but I hasten to add I don't mean that as an informed opinion of what should be done. It's just that what routing foo I have does not encompass routes via interfaces that don't have an address/mask that allow access to that ip address. Like I said, weird. As far as I can tell, there's no authoritative/standard document that describes this special case, and ISC DHCP only applies this special case to the default gateway IP specified by DHO_ROUTERS. Notably, it does *not* apply this direct routing logic for gateway IPs specified by DHO_STATIC_ROUTES and/or DHO_CLASSLESS_STATIC_ROUTES, but it's unclear whether that's intentional or accidental. Not from me. This is way bigger that the last three line diff, and I have insufficient routing foo to comment on the usefulness of all these routes. The original three line diff was so short because it mimicked ISC DHCP and only special cased the default gateway (i.e., DHO_ROUTERS), and only did so when DHO_ROUTERS was actually used (i.e., when DHO_CLASSLESS_STATIC_ROUTES is not specified). I was incorrectly reading your desire as wanting to add a route to the address being bound, not a route to the gateway. I also didn't understand why you wanted to do *that*, but it seemed a small, contained change. All the added complexity to attempt to address my misunderstanding was not needed. Sorry about the red herring! You suggested we should unconditionally add the route, but it was unclear to me whether you meant: 1. We should always apply the special case logic for adding a direct route for the default gateway specified by DHO_ROUTERS, even if DHO_CLASSLESS_STATIC_ROUTES is specified; or 2. We should apply the special case logic to add direct routes for all gateways (i.e., those specified by DHO_CLASSLESS_STATIC_ROUTES and DHO_STATIC_ROUTES too), not just the default gateway. Interpretation #1 didn't seem right, because if DHO_CLASSLESS_STATIC_ROUTES is specified, then DHO_ROUTERS isn't used at all, so it might not be meaningful to add a direct route for DHO_ROUTERS's gateway. Interpretation #2 seems reasonable (though deviating from ISC DHCP's undocumented behavior), so that's what the second patch implemented. But that's necessarily more code than the original diff, because we need to repeat the special case logic for each gateway IP as they're extracted from the DHCP options. The change is somewhat bigger and more invasive, but I think not that much more complex: 1. Add a function ensure_direct_route() function that applies ISC DHCP's /32 logic for a given gateway IP address (same code as from first patch). 2. Call ensure_direct_route() in each of add_default_route(), add_static_routes(), and add_classless_static_routes() for each gateway IP address. 3. Add extra 'addr' and 'addrmask' parameters to each of the add_*_routes() functions to pass the extra data needed for ensure_direct_route(), and fix call sites appropriately. 4. In add_static_routes(), rename the local 'addr' variable that points into option data to 'data' to avoid conflicting with the new 'addr' parameter. Given the above, I think the original change makes the most sense. Ken
ugl(4): self-dv_unit fix (Re: cdce(4): sc-cdce_unit)
Hello,
if_ugl.c has similar issue, and I think this should be fixed.
ok to commit?
SASANO Takayoshi u...@mx5.nisiq.net
Index: if_ugl.c
===
RCS file: /cvs/src/sys/dev/usb/if_ugl.c,v
retrieving revision 1.2
diff -u -p -r1.2 if_ugl.c
--- if_ugl.c3 Dec 2013 21:06:59 - 1.2
+++ if_ugl.c5 Dec 2013 20:06:50 -
@@ -152,7 +152,6 @@ struct ugl_softc {
uByte sc_ibuf[UGL_INTR_PKTLEN];
- int sc_unit;
u_int sc_rx_errs;
struct timeval sc_rx_notice;
u_int sc_intr_errs;
@@ -246,7 +245,6 @@ ugl_attach(struct device *parent, struct
return;
}
- sc-sc_unit = self-dv_unit;
sc-sc_udev = dev;
sc-sc_product = uaa-product;
sc-sc_vendor = uaa-vendor;
@@ -292,7 +290,7 @@ ugl_attach(struct device *parent, struct
macaddr_hi = htons(0x2acb);
bcopy(macaddr_hi, sc-sc_arpcom.ac_enaddr[0], sizeof(u_int16_t));
bcopy(ticks, sc-sc_arpcom.ac_enaddr[2], sizeof(u_int32_t));
- sc-sc_arpcom.ac_enaddr[5] = (u_int8_t)(sc-sc_unit);
+ sc-sc_arpcom.ac_enaddr[5] = (u_int8_t)(sc-sc_dev.dv_unit);
printf(%s: address %s\n,
sc-sc_dev.dv_xname, ether_sprintf(sc-sc_arpcom.ac_enaddr));
Re: LLVM warning in dev/ic/uhci.c
On Wed, Dec 04, 2013 at 05:04:13PM +0100, Mark Kettenis wrote:
Date: Wed, 4 Dec 2013 12:53:20 +0100
From: Joerg Sonnenberger jo...@britannica.bec.de
On Wed, Dec 04, 2013 at 12:24:50PM +0100, Mark Kettenis wrote:
Sorry, but I disagree with LLVM here. It shouldn't complain about
static inline functions.
Then mark them as unused, just like you would with a static variable you
insist on keeping.
Thanks Joerg, that might work.
Brad, does LLVM like the diff below? Note this changes ohci.c not
uhci.c since I happened to have some uncommtted changes in the latter
that I didn't want to touch.
Index: ohci.c
===
RCS file: /cvs/src/sys/dev/usb/ohci.c,v
retrieving revision 1.116
diff -u -p -r1.116 ohci.c
--- ohci.c9 Nov 2013 08:46:05 - 1.116
+++ ohci.c4 Dec 2013 16:01:42 -
@@ -189,21 +189,21 @@ voidohci_dump_itds(struct ohci_soft_it
#define OWRITE4(sc, r, x) \
do { OBARR(sc); bus_space_write_4((sc)-iot, (sc)-ioh, (r), (x)); } while
(0)
-static __inline u_int8_t
+__unused static __inline u_int8_t
OREAD1(struct ohci_softc *sc, bus_size_t r)
{
OBARR(sc);
return bus_space_read_1(sc-iot, sc-ioh, r);
}
-static __inline u_int16_t
+__unused static __inline u_int16_t
OREAD2(struct ohci_softc *sc, bus_size_t r)
{
OBARR(sc);
return bus_space_read_2(sc-iot, sc-ioh, r);
}
-static __inline u_int32_t
+__unused static __inline u_int32_t
OREAD4(struct ohci_softc *sc, bus_size_t r)
{
OBARR(sc);
Yes, this does indeed silence the warning. So you want something like the
following?
Index: ohci.c
===
RCS file: /home/cvs/src/sys/dev/usb/ohci.c,v
retrieving revision 1.116
diff -u -p -r1.116 ohci.c
--- ohci.c 9 Nov 2013 08:46:05 - 1.116
+++ ohci.c 5 Dec 2013 20:41:53 -
@@ -189,21 +189,21 @@ void ohci_dump_itds(struct ohci_soft_it
#define OWRITE4(sc, r, x) \
do { OBARR(sc); bus_space_write_4((sc)-iot, (sc)-ioh, (r), (x)); } while (0)
-static __inline u_int8_t
+__unused static __inline u_int8_t
OREAD1(struct ohci_softc *sc, bus_size_t r)
{
OBARR(sc);
return bus_space_read_1(sc-iot, sc-ioh, r);
}
-static __inline u_int16_t
+__unused static __inline u_int16_t
OREAD2(struct ohci_softc *sc, bus_size_t r)
{
OBARR(sc);
return bus_space_read_2(sc-iot, sc-ioh, r);
}
-static __inline u_int32_t
+__unused static __inline u_int32_t
OREAD4(struct ohci_softc *sc, bus_size_t r)
{
OBARR(sc);
Index: uhci.c
===
RCS file: /home/cvs/src/sys/dev/usb/uhci.c,v
retrieving revision 1.104
diff -u -p -r1.104 uhci.c
--- uhci.c 9 Nov 2013 08:46:05 - 1.104
+++ uhci.c 5 Dec 2013 20:42:24 -
@@ -238,21 +238,21 @@ void uhci_dump(void);
do { UBARR(sc); bus_space_write_4((sc)-iot, (sc)-ioh, (r), (x)); \
} while (/*CONSTCOND*/0)
-static __inline u_int8_t
+__unused static __inline u_int8_t
UREAD1(struct uhci_softc *sc, bus_size_t r)
{
UBARR(sc);
return bus_space_read_1(sc-iot, sc-ioh, r);
}
-static __inline u_int16_t
+__unused static __inline u_int16_t
UREAD2(struct uhci_softc *sc, bus_size_t r)
{
UBARR(sc);
return bus_space_read_2(sc-iot, sc-ioh, r);
}
-static __inline u_int32_t
+__unused static __inline u_int32_t
UREAD4(struct uhci_softc *sc, bus_size_t r)
{
UBARR(sc);
--
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.
Re: LLVM warning in dev/pci/if_wpi.c
On Tue, Dec 03, 2013 at 08:31:13PM -0500, Brad Smith wrote:
An unused function in wpi(4). #if 0 out the function for now.
if_wpi.c:510:1: error: unused function 'wpi_mem_write'
[-Werror,-Wunused-function]
OK?
Index: if_wpi.c
===
RCS file: /home/cvs/src/sys/dev/pci/if_wpi.c,v
retrieving revision 1.116
diff -u -p -r1.116 if_wpi.c
--- if_wpi.c 28 Nov 2013 20:07:32 - 1.116
+++ if_wpi.c 4 Dec 2013 00:55:43 -
@@ -506,6 +506,7 @@ wpi_mem_read(struct wpi_softc *sc, uint3
return WPI_READ(sc, WPI_MEM_RDATA);
}
+#if 0
static __inline void
wpi_mem_write(struct wpi_softc *sc, uint32_t addr, uint32_t data)
{
@@ -513,6 +514,7 @@ wpi_mem_write(struct wpi_softc *sc, uint
WPI_BARRIER_WRITE(sc);
WPI_WRITE(sc, WPI_MEM_WDATA, data);
}
+#endif
static __inline void
wpi_mem_read_region_4(struct wpi_softc *sc, uint32_t addr, uint32_t *data,
I'm guessing kettenis will probably want something like this instead.
OK?
Index: if_wpi.c
===
RCS file: /home/cvs/src/sys/dev/pci/if_wpi.c,v
retrieving revision 1.116
diff -u -p -r1.116 if_wpi.c
--- if_wpi.c28 Nov 2013 20:07:32 - 1.116
+++ if_wpi.c5 Dec 2013 20:58:49 -
@@ -506,7 +506,7 @@ wpi_mem_read(struct wpi_softc *sc, uint3
return WPI_READ(sc, WPI_MEM_RDATA);
}
-static __inline void
+__unused static __inline void
wpi_mem_write(struct wpi_softc *sc, uint32_t addr, uint32_t data)
{
WPI_WRITE(sc, WPI_MEM_WADDR, addr);
--
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.
