Changeset View
Standalone View
sbin/ifconfig/ifpfsync.c
Context not available. | |||||
setpfsync_syncpeer(const char *val, int d, int s, const struct afswtch *rafp) | setpfsync_syncpeer(const char *val, int d, int s, const struct afswtch *rafp) | ||||
{ | { | ||||
struct pfsyncreq preq; | struct pfsyncreq preq; | ||||
struct addrinfo hints, *peerres; | struct addrinfo *peerres; | ||||
int ecode; | int ecode; | ||||
bzero((char *)&preq, sizeof(struct pfsyncreq)); | bzero((char *)&preq, sizeof(struct pfsyncreq)); | ||||
Context not available. | |||||
if (ioctl(s, SIOCGETPFSYNC, (caddr_t)&ifr) == -1) | if (ioctl(s, SIOCGETPFSYNC, (caddr_t)&ifr) == -1) | ||||
err(1, "SIOCGETPFSYNC"); | err(1, "SIOCGETPFSYNC"); | ||||
memset(&hints, 0, sizeof(hints)); | if ((ecode = getaddrinfo(val, NULL, NULL, &peerres)) != 0) | ||||
hints.ai_family = AF_INET; | |||||
hints.ai_socktype = SOCK_DGRAM; /*dummy*/ | |||||
if ((ecode = getaddrinfo(val, NULL, &hints, &peerres)) != 0) | |||||
errx(1, "error in parsing address string: %s", | errx(1, "error in parsing address string: %s", | ||||
gai_strerror(ecode)); | gai_strerror(ecode)); | ||||
if (peerres->ai_addr->sa_family != AF_INET) | switch (peerres->ai_family) { | ||||
errx(1, "only IPv4 addresses supported for the syncpeer"); | #ifdef INET | ||||
case AF_INET: { | |||||
struct sockaddr_in *sin = (struct sockaddr_in *)peerres->ai_addr; | |||||
if (IN_MULTICAST(ntohl(sin->sin_addr.s_addr))) | |||||
errx(1, "syncpeer address cannot be multicast"); | |||||
preq.pfsyncr_syncpeer.s_addr = ((struct sockaddr_in *) | preq.pfsyncr_syncpeer.in4 = *sin; | ||||
peerres->ai_addr)->sin_addr.s_addr; | break; | ||||
} | |||||
#endif | |||||
kp: Is this worth considering? Do we every specify something that's not IPv4 or IPv6? | |||||
Done Inline ActionsThe way pfsync was handling the case where no peer address was set (or was unset) was by leaving the address empty (that would later be filled by the multicast address in if_pfsync.c), which would result in a AF that is neither IPv4 nor IPv6. email_luiz.eng.br: The way pfsync was handling the case where no peer address was set (or was unset) was by… | |||||
default: | |||||
errx(1, "syncpeer address %s not supported", val); | |||||
} | |||||
if (ioctl(s, SIOCSETPFSYNC, (caddr_t)&ifr) == -1) | if (ioctl(s, SIOCSETPFSYNC, (caddr_t)&ifr) == -1) | ||||
err(1, "SIOCSETPFSYNC"); | err(1, "SIOCSETPFSYNC"); | ||||
Not Done Inline ActionsThis might be something that can be usefully translated into a libpfctl library function. kp: This might be something that can be usefully translated into a libpfctl library function. | |||||
Context not available. | |||||
if (ioctl(s, SIOCGETPFSYNC, (caddr_t)&ifr) == -1) | if (ioctl(s, SIOCGETPFSYNC, (caddr_t)&ifr) == -1) | ||||
err(1, "SIOCGETPFSYNC"); | err(1, "SIOCGETPFSYNC"); | ||||
preq.pfsyncr_syncpeer.s_addr = 0; | switch (preq.pfsyncr_syncpeer.sa.sa_family) { | ||||
#ifdef INET | |||||
Not Done Inline ActionsThis seems like pointless work. Can't we just nvlist_add_string(nvl, "synced", val);? kp: This seems like pointless work. Can't we just `nvlist_add_string(nvl, "synced", val);`? | |||||
Done Inline ActionsIt was a complicated way of ensuring val was not bigger than IFNAMSIZ. I added a check for the size, and adjusted the code per your suggestion. if (strlen(val) > IFNAMSIZ) errx(1, "interface name %s is too long", val); email_luiz.eng.br: It was a complicated way of ensuring val was not bigger than IFNAMSIZ.
I added a check for… | |||||
case AF_INET: | |||||
#endif | |||||
{ | |||||
bzero((char *)&preq.pfsyncr_syncpeer, | |||||
glebiusUnsubmitted Done Inline ActionsWe prefer POSIX memset() over bzero() in new code. glebius: We prefer POSIX memset() over bzero() in new code. | |||||
sizeof(union pfsync_sockaddr)); | |||||
break; | |||||
} | |||||
} | |||||
if (ioctl(s, SIOCSETPFSYNC, (caddr_t)&ifr) == -1) | if (ioctl(s, SIOCSETPFSYNC, (caddr_t)&ifr) == -1) | ||||
err(1, "SIOCSETPFSYNC"); | err(1, "SIOCSETPFSYNC"); | ||||
Not Done Inline ActionsIs it worth doing this? All we want is to set an empty string for syncdev, so why not just do that unconditionally? kp: Is it worth doing this? All we want is to set an empty string for syncdev, so why not just do… | |||||
Done Inline ActionsI will simplify it, but we still need the nvlist_exists_string() check, otherwise I get a EEXIST error, as the syncdev is returned by the SIOCGETPFSYNCNV ioctl. email_luiz.eng.br: I will simplify it, but we still need the `nvlist_exists_string()` check, otherwise I get a… | |||||
Not Done Inline ActionsAlso a good libpfctl candidate. kp: Also a good libpfctl candidate. | |||||
Context not available. | |||||
pfsync_status(int s) | pfsync_status(int s) | ||||
Done Inline ActionsWe should try to be consistent about using {} for single line statements. At least within the same function. kp: We should try to be consistent about using {} for single line statements. At least within the… | |||||
{ | { | ||||
struct pfsyncreq preq; | struct pfsyncreq preq; | ||||
char syncpeer[NI_MAXHOST]; | |||||
struct sockaddr *syncpeer_sa; | |||||
int error; | |||||
Not Done Inline ActionsAnd we'd do case AF_INET6 here, right? kp: And we'd do case AF_INET6 here, right?
Which will come in a subsequent commit? | |||||
Done Inline ActionsThat is my current plan. email_luiz.eng.br: That is my current plan. | |||||
bzero((char *)&preq, sizeof(struct pfsyncreq)); | bzero((char *)&preq, sizeof(struct pfsyncreq)); | ||||
ifr.ifr_data = (caddr_t)&preq; | ifr.ifr_data = (caddr_t)&preq; | ||||
syncpeer_sa = &preq.pfsyncr_syncpeer.sa; | |||||
if (ioctl(s, SIOCGETPFSYNC, (caddr_t)&ifr) == -1) | if (ioctl(s, SIOCGETPFSYNC, (caddr_t)&ifr) == -1) | ||||
return; | return; | ||||
if (preq.pfsyncr_syncdev[0] != '\0' || | if (preq.pfsyncr_syncdev[0] != '\0' || | ||||
preq.pfsyncr_syncpeer.s_addr != htonl(INADDR_PFSYNC_GROUP)) | syncpeer_sa->sa_family != AF_UNSPEC) | ||||
printf("\t"); | printf("\t"); | ||||
if (preq.pfsyncr_syncdev[0] != '\0') | if (preq.pfsyncr_syncdev[0] != '\0') | ||||
printf("pfsync: syncdev: %s ", preq.pfsyncr_syncdev); | printf("syncdev: %s ", preq.pfsyncr_syncdev); | ||||
if (preq.pfsyncr_syncpeer.s_addr != htonl(INADDR_PFSYNC_GROUP)) | |||||
printf("syncpeer: %s ", inet_ntoa(preq.pfsyncr_syncpeer)); | if (preq.pfsyncr_syncpeer.sa.sa_family != AF_UNSPEC && | ||||
preq.pfsyncr_syncpeer.in4.sin_addr.s_addr != htonl(INADDR_PFSYNC_GROUP)) { | |||||
if ((error = getnameinfo(syncpeer_sa, syncpeer_sa->sa_len, | |||||
syncpeer, sizeof(syncpeer), NULL, 0, NI_NUMERICHOST)) != 0) | |||||
errx(1, "getnameinfo: %s", gai_strerror(error)); | |||||
printf("syncpeer: %s ", syncpeer); | |||||
} | |||||
if (preq.pfsyncr_syncdev[0] != '\0' || | if (preq.pfsyncr_syncdev[0] != '\0' || | ||||
preq.pfsyncr_syncpeer.s_addr != htonl(INADDR_PFSYNC_GROUP)) { | (preq.pfsyncr_syncpeer.sa.sa_family != AF_UNSPEC && | ||||
preq.pfsyncr_syncpeer.in4.sin_addr.s_addr != htonl(INADDR_PFSYNC_GROUP))) { | |||||
printf("maxupd: %d ", preq.pfsyncr_maxupdates); | printf("maxupd: %d ", preq.pfsyncr_maxupdates); | ||||
printf("defer: %s\n", | printf("defer: %s\n", | ||||
(preq.pfsyncr_defer & PFSYNCF_DEFER) ? "on" : "off"); | (preq.pfsyncr_defer & PFSYNCF_DEFER) ? "on" : "off"); | ||||
Context not available. | |||||
Done Inline ActionsYes. That's a status flag, and it'll get reset by the kernel on every read. kp: Yes. That's a status flag, and it'll get reset by the kernel on every read.
We can set or clear… | |||||
Not Done Inline ActionsWhat's the purpose of pfsync_kstatus? The 'k' prefix usual means this is a kernel-only structure (at least in the context of pf), so I'm a little confused at seeing that in user space. This might be a structure definition that belongs in libpfctl.h kp: What's the purpose of pfsync_kstatus?
The 'k' prefix usual means this is a kernel-only… | |||||
Not Done Inline ActionsI tried to mimic what was done with pf_kstate in pf_ioctl.c but failed to understand it was a kernel-only structure. I made this function considering that it would be useful for the other functions, but in the end it only got used once in pfsync_status. I'm considering getting rid of it and moving this code into pfsync_status directly. email_luiz.eng.br: I tried to mimic what was done with pf_kstate in pf_ioctl.c but failed to understand it was a… | |||||
Done Inline ActionsWe should zero out the status struct so we don't end up with random garbage if the kernel doesn't supply a syncdev. kp: We should zero out the status struct so we don't end up with random garbage if the kernel… | |||||
Done Inline ActionsSince we now zero out the status struct here, I removed the zeroing out that was done in line 367, before calling pfsync_nvstatus_to_kstatus email_luiz.eng.br: Since we now zero out the status struct here, I removed the zeroing out that was done in line… | |||||
Not Done Inline ActionsShould we be returning an error here? It's arguable, because we're checking the result of pfsync_syncpeer_nvlist_to_sockaddr(). On the other hand, the kernel really shouldn't be sending us invalid data. Perhaps this needs an assertion. kp: Should we be returning an error here?
It's arguable, because we're checking the result of… | |||||
Not Done Inline ActionsI looked around the ifconfig code and it seems they don't use assertions. I'm inclined towards returning an error. email_luiz.eng.br: I looked around the ifconfig code and it seems they don't use assertions. I'm inclined towards… | |||||
Done Inline ActionsWe leak the nvlist if the ioctl fails. kp: We leak the nvlist if the ioctl fails. |
Is this worth considering? Do we every specify something that's not IPv4 or IPv6?