Daniel Veillard <veillard@xxxxxxxxxx> wrote on 02/11/2010 06:08:15 AM:
[...]
> > +int openMacvtapTap(virConnectPtr conn,
> > + const char *ifname,
> > + const unsigned char *macaddress,
> > + const char *linkdev,
> > + int mode,
> > + char **res_ifname);
> > +
> > +int delMacvtapByMACAddress(virConnectPtr conn,
> > + const unsigned char *macaddress,
> > + int *hasBusyDev);
> > +
> > +#endif /* WITH_MACVTAP */
> > +
> > +#define MACVTAP_MODE_PRIVATE_STR "private"
> > +#define MACVTAP_MODE_VEPA_STR "vepa"
> > +#define MACVTAP_MODE_BRIDGE_STR "bridge"
> > +
> > +
> > +#endif
>
> minor suggestion: #endif /* __UTIL_MACVTAP_H__ */
Fixed in upcoming patch.
[...]
> > +
> > +#define VIR_FROM_THIS VIR_FROM_NONE
> > +
> > +#define ReportError(conn, code, fmt...) \
> > + virReportErrorHelper(conn, VIR_FROM_NONE, code, __FILE__, \
> > + __FUNCTION__, __LINE__, fmt)
>
> Hum, I would suggest to use VIR_FROM_NET instead of VIR_FROM_NONE,
> or add a specific virErrorDomain enum in virterror.h (and associated
> code in virterror.c). I think VIR_FROM_NET is the simplest for now.
Fixed in upcoming patch.
>
> > +#define MACVTAP_NAME_PREFIX "macvtap"
> > +#define MACVTAP_NAME_PATTERN "macvtap%d"
> > +
> > +static int nlOpen(virConnectPtr conn)
> > +{
> > + int fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
> > + if (fd < 0)
> > + virReportSystemError(conn, errno,
> > + "%s",_("cannot open netlink socket"));
> > + return fd;
> > +}
> > +
> the function signature will change in the rebase as others in the code
> too
True.
>
> > +static void nlClose(int fd)
> > +{
> > + close(fd);
> > +}
> > +
> > +
> > +static
> > +int nlComm(virConnectPtr conn,
> > + struct nlmsghdr *nlmsg,
> > + char *respbuf, int *respbuflen)
> > +{
> > + int rc = 0;
> > + struct sockaddr_nl nladdr = {
> > + .nl_family = AF_NETLINK,
> > + .nl_pid = 0,
> > + .nl_groups = 0,
> > + };
> > + char rcvbuf[1024];
> > + ssize_t nbytes;
> > + size_t tocopy;
> > + int fd = nlOpen(conn);
> > +
> > + if (fd < 0)
> > + return -1;
> > +
> > + nlmsg->nlmsg_flags |= NLM_F_ACK;
> > +
> > + nbytes = sendto(fd, (void *)nlmsg, nlmsg->nlmsg_len, 0,
> > + (struct sockaddr *)&nladdr, sizeof(nladdr));
> > + if (nbytes < 0) {
> > + virReportSystemError(conn, errno,
> > + "%s", _("cannot send to netlink socket"));
> > + rc = -1;
> > + goto err_exit;
> > + }
> > +
> > + memset(rcvbuf, 0x0, sizeof(rcvbuf));
> > + while (1) {
> > + socklen_t addrlen = sizeof(nladdr);
> > + nbytes = recvfrom(fd, &rcvbuf, sizeof(rcvbuf), 0,
> > + (struct sockaddr *)&nladdr, &addrlen);
> > + if (nbytes < 0) {
> > + if (errno == EAGAIN || errno == EINTR)
> > + continue;
> > + virReportSystemError(conn, errno, "%s",
> > + _("error receiving from netlink socket"));
> > + rc = -1;
> > + goto err_exit;
> > + }
> > +
> > + tocopy = (nbytes < *respbuflen) ? nbytes : *respbuflen;
> > + memcpy(respbuf, rcvbuf, tocopy);
> > + *respbuflen = tocopy;
> > + break;
> > + }
> > +
> > +err_exit:
> > + nlClose(fd);
> > + return rc;
> > +}
>
> Hum ... I don't see why we need rcvbuf here, we could make the recvfrom
> directly from respbuf with the respbuflen size, and no need to allocate
> one KB on stack. Also if for some reason one expect a large response
> packet there caller will be in control instead of a fixed size arbitrary
> limit in the function. The only case I could come to justifying this is
> if the caller want to truncate the response or if netlink force using
> a 1KB buffer input size.
In the next patch I'll write into the provided receive buffer.
>
> [...]
> > +static int
> > +link_add(virConnectPtr conn,
> > + const char *type,
> > + const unsigned char *macaddress, int macaddrsize,
> > + const char *ifname,
> > + const char *srcdev,
> > + uint32_t macvlan_mode,
> > + int *retry)
> > +{
> > + char nlmsgbuf[1024], recvbuf[1024];
> > + struct nlmsghdr *nlm = (struct nlmsghdr *)nlmsgbuf, *resp;
> > + struct nlmsgerr *err;
> > + char rtattbuf[256];
> > + struct rtattr *rta, *rta1, *li;
> > + struct ifinfomsg i = { .ifi_family = AF_UNSPEC };
[...]
> > +
> > + if (macvlan_mode > 0) {
> > + rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_INFO_DATA,
> > + NULL, 0);
> > + if (!rta)
> > + goto buffer_too_small;
> > +
> > + if (!(rta1 = nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf,
> rta->rta_len)))
> > + goto buffer_too_small;
> > +
> > + rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_MACVLAN_MODE,
> > + &macvlan_mode, sizeof(macvlan_mode));
> > + if (!rta)
> > + goto buffer_too_small;
> > +
> > + if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
> > + goto buffer_too_small;
> > +
> > + rta1->rta_len = (char *)nlm + nlm->nlmsg_len - (char *)rta1;
> > + }
> > +
> > + li->rta_len = (char *)nlm + nlm->nlmsg_len - (char *)li;
>
> I must admit that I'm again a bit surprized by the buffer handling.
> There is a function to append stuff on a buffer, so all this code could
> be changed to use dymanic allocation easilly and not be stuck on a fixed
> buffer size allocated on stack (a couple of buffers even). since we call
> nlComm below, we already have 3KB of stack allocated buffers piled up
> and honnestly none of this is required as far as I understand.
The thing with the netlink messages is that their content needs to be 'nested', meaning that you
add data to a netlink message parameter, depending on availability of parameters in the function,
i.e., test for macvlan_mode > 0, and only later on you can determine how large the size of the
nested information was. Above I am setting the li pointer in the nlAppend() call. Later on only
I set the size of the nested information in the li->rta_len = ... line. So the pointer for li better
not have changed while reallocating the target message buffer for example.
It's true that the size of the buffers is rather generous. I could limit them to around 128 bytes if
that's better. At least that is sufficient.
> > +
> > + snprintf(path, sizeof(path), "/sys/class/net/%s/ifindex", ifname);
>
> virBuildPathInternal could be another way, in any case one should
> check the return value
Checking return value in next patch.
>
> > + file = fopen(path, "r");
> > +
> > + if (!file) {
> > + virReportSystemError(conn, errno,
> > + _("cannot open macvtap file %s to determine "
> > + "interface index"), path);
> > + return -1;
> > + }
> > +
> > + if (fscanf(file, "%d", &ifindex) != 1) {
> > + virReportSystemError(conn, errno,
> > + "%s",_("cannot determine macvtap's
> tap device "
> > + "interface index"));
> > + fclose(file);
> > + return -1;
> > + }
> > +
> > + fclose(file);
> > +
> > + snprintf(tapname, sizeof(tapname), "/dev/tap%d", ifindex);
> > +
> > + while (1) {
> > + // may need to wait for udev to be done
> > + tapfd = open(tapname, O_RDWR);
> > + if (tapfd < 0 && retries--) {
> > + usleep(20000);
> > + continue;
> > + }
> > + break;
> > + }
>
> hum, the function should check retries > 0 argument otherwise this
> could hang the daemon.
Changed this.
>
> > + if (tapfd < 0)
> > + virReportSystemError(conn, errno,
> > + _("cannot open macvtap tap device %s"),
> > + tapname);
> > +
> > + return tapfd;
> > +}
>
> okay, I'm not too convinced by the buffer management code, but that
> could be cleaned up in a later pach, so ACK
I'll shrink the sizes of the buffers. Want to be careful about dynamic allocation
of the buffers in this case, though.
Stefan
>
> Daniel
>
> --
> Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
> daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
> http://veillard.com/ | virtualization library http://libvirt.org/
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list