On Thu, Feb 11, 2010 at 08:14:17AM -0500, Stefan Berger wrote:
> Daniel Veillard <veillard@xxxxxxxxxx> wrote on 02/11/2010 06:08:15 AM:
[...]
> > 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.
okay thanks !
> > [...]
> > > +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.
It's just that a dynamic allocation, growing it on demand was looking
more natural actually, you don't know the size a priori.
For example you could use a virBuffer structure and use
virBufferAdd(buf, input, len)
this would do the allocation as needed, and you would just have to check
for error with virBufferError() once after the set of operations before
calling the netlink sending function.
> > > +
> > > + 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.
okay
> >
> > > + 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.
thanks !
> >
> > > + 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.
Or just make it dynamic, reusing our existing library code for this,
as said I don't think it's a blocker but that would ease things on the
long term I guess,
thanks,
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