libvir-list-bounces@redhat.com wrote on 05/25/2010
09:00:26 AM:
>
> Scott Feldman wrote:
> > From: Scott Feldman <scofeldm@cisco.com>
> ...
> > diff --git a/configure.ac b/configure.ac
> ...
> > -if test "$with_macvtap" = "yes"; then
> > +if test "$with_macvtap" = "yes" || "$with_virtualport"
= "yes"; then
>
> Hi Scott,
>
> The above introduces a syntax error.
> Fix it by inserting a "test" after the "||":
My bad. Will fix it.
> > +static int
> > +nlCommWaitSuccess(struct nlmsghdr *nlmsg, int nl_groups,
> > + char
**respbuf, int *respbuflen, long to_usecs)
>
> The parameters, respbuflen and nl_groups make sense only when
> non-negative, so my reflex is to make their types reflect that.
> Any reason not to use an unsigned type in those cases?
nl_groups should be uint32_t or even __u32 to reflect
the kernel includes.
I'll fix them.
>
> This "to_usecs" parameter is in the same boat, in that it
is
> logically non-negative. I suggest using unsigned long long
> for it, since technically (with the two longs in struct timeval)
> a portable timeout can be as large as 2^31 * 1000^2 microseconds.
I'll do that.
> Also, it'd be nice to rename it to "timeout_usecs", since
"to_usecs"
> made me think of a flag that says whether to convert "to microseconds".
>
Will rename the variable.
> > +{
> > + int rc = 0;
> > + struct sockaddr_nl nladdr = {
> > + .nl_family = AF_NETLINK,
> > + .nl_pid =
getpid(),
> > + .nl_groups = nl_groups,
> > + };
> > + int rcvChunkSize = 1024; // expecting less than
that
> > + int rcvoffset = 0;
>
> This should be of type size_t.
Ok.
> It is used as an accumulator, and we do add nbytes (of type ssize_t)
to it,
> and (esp!) use it as an array index, so its type must be unsigned.
> Otherwise, overflow could be exploitable.
>
> > + ssize_t nbytes;
> > + int n;
> > + struct timeval tv = {
> > + .tv_sec = to_usecs / MICROSEC_PER_SEC,
> > + .tv_usec = to_usecs % MICROSEC_PER_SEC,
> > + };
> > + fd_set rfds;
>
> At least "n" and "rfds" are used only in the while
loop,
> so their declarations belong "down" there, too.
Sure, I'll move them.
>
> > + bool gotvalid = false;
>
> Personal preference:
> I find that a name like "got_valid" is easier to read than
a
> variablenamewithnoseparatorbetweenwords.
> Same for rcvoffset vs. rcv_offset, etc.
Ok. I can rename them as well.
>
> > + int fd = nlOpen();
> > + static uint32_t seq = 0x1234;
> > + uint32_t myseq = seq++;
> > + uint32_t mypid = getpid();
> > +
> > + if (fd < 0)
> > + return -1;
> > +
> > + nlmsg->nlmsg_pid = mypid;
> > + nlmsg->nlmsg_seq = myseq;
> > + nlmsg->nlmsg_flags |= NLM_F_ACK;
> > +
> > + nbytes = sendto(fd, (void *)nlmsg, nlmsg->nlmsg_len,
0,
> > +
(struct sockaddr *)&nladdr, sizeof(nladdr));
> > + if (nbytes < 0) {
> > + virReportSystemError(errno,
> > +
"%s", _("cannot send
to netlink socket"));
> > + rc = -1;
> > + goto err_exit;
> > + }
> > +
> > + while (!gotvalid) {
> > + rcvoffset = 0;
> > + while (1) {
> > + socklen_t addrlen
= sizeof(nladdr);
> > +
> > + if (VIR_REALLOC_N(*respbuf,
rcvoffset+rcvChunkSize) < 0) {
> > + virReportOOMError();
> > + rc =
-1;
> > + goto
err_exit;
> > + }
> > +
> > + FD_ZERO(&rfds);
> > + FD_SET(fd, &rfds);
> > +
> > + n = select(fd + 1,
&rfds, NULL, NULL, &tv);
> > + if (n == 0) {
>
> That handles the case of an expired timeout.
> However, if select fails, it returns -1.
> You should diagnose that failure rather than going
> ahead and reading from "fd".
I'll dump an error and will return in case of -1.
>
> > + rc =
-1;
> > + goto
err_exit;
> > + }
> > +
> > + nbytes = recvfrom(fd,
&((*respbuf)[rcvoffset]),
> rcvChunkSize, 0,
> > +
(struct sockaddr *)&nladdr,
&addrlen);
> > + if (nbytes < 0)
{
> > + if (errno
== EAGAIN || errno == EINTR)
> > +
continue;
> > + virReportSystemError(errno,
"%s",
> > +
_("error
receiving from
> netlink socket"));
> > + rc =
-1;
> > + goto
err_exit;
> > + }
> > + rcvoffset += nbytes;
> > + break;
> > + }
> > + *respbuflen = rcvoffset;
> > +
> > + /* check message for error */
> > + if (*respbuflen > NLMSG_LENGTH(0)
&& *respbuf != NULL) {
> > + struct nlmsghdr *resp
= (struct nlmsghdr *)*respbuf;
> > + struct nlmsgerr *err;
> > +
> > + if (resp->nlmsg_pid
!= mypid ||
> > + resp->nlmsg_seq
!= myseq)
> > + continue;
> > +
> > + /* skip reflected
message */
> > + if (resp->nlmsg_type
& 0x10)
> > + continue;
> > +
> > + switch (resp->nlmsg_type)
{
> > + case NLMSG_ERROR:
> > + err
= (struct nlmsgerr *)NLMSG_DATA(resp);
> > + if
(resp->nlmsg_len >= NLMSG_LENGTH(sizeof(*err))) {
> > +
if (-err->error != EOPNOTSUPP) {
>
> I'm used to seeing this:
>
if (err->error != -EOPNOTSUPP) {
> why do you do it the other way?
> Such differences are generally discouraged.
Alright. Will fix it.
>
> > +
/* assuming error msg from daemon */
> > +
gotvalid = true;
> > +
break;
> > +
}
> > + }
> > + /*
whatever this is, skip it */
> > + VIR_FREE(*respbuf);
> > + *respbuf
= NULL;
>
> The above is a dead store, since VIR_FREE has just done that.
Ok.
>
> > + *respbuflen
= 0;
> > + break;
> > +
> > + case NLMSG_DONE:
> > + gotvalid
= true;
> > + break;
> > +
> > + default:
> > + VIR_FREE(*respbuf);
> > + *respbuf
= NULL;
>
> Likewise. Remove the dead store.
Ok.
>
> > + *respbuflen
= 0;
> > + break;
> > + }
> > + }
> > + }
> > +
> > +err_exit:
> > + if (rc == -1) {
> > + VIR_FREE(*respbuf);
> > + *respbuf = NULL;
>
> Likewise. Remove the dead store.
OK.
>
> > + *respbuflen = 0;
> > + }
> > +
> > + nlClose(fd);
> > + return rc;
> > +}
> > +
> > +# endif
> ...
>
> > +static int
> > +link_dump(int ifindex, struct nlattr **tb, char **recvbuf)
> > +{
> > + int rc = 0;
> > + char nlmsgbuf[256] = { 0, };
> > + struct nlmsghdr *nlm = (struct nlmsghdr *)nlmsgbuf,
*resp;
> > + struct nlmsgerr *err;
> > + struct ifinfomsg i = {
> > + .ifi_family = AF_UNSPEC,
> > + .ifi_index = ifindex
> > + };
>
> When I first read the "sizeof(i)" below I did a double take.
> I'd prefer a name that does not make me think of an integer index.
Will fix this.
>
> > + int recvbuflen;
> > +
> > + *recvbuf = NULL;
> > +
> > + nlInit(nlm, NLM_F_REQUEST, RTM_GETLINK);
> > +
> > + if (!nlAppend(nlm, sizeof(nlmsgbuf), &i, sizeof(i)))
> > + goto buffer_too_small;
> > +
> > + if (nlComm(nlm, recvbuf, &recvbuflen) <
0)
> > + return -1;
> > +
> > + if (recvbuflen < NLMSG_LENGTH(0) || *recvbuf
== NULL)
> > + goto malformed_resp;
> > +
> > + resp = (struct nlmsghdr *)*recvbuf;
> > +
> > + switch (resp->nlmsg_type) {
> > + case NLMSG_ERROR:
> > + err = (struct nlmsgerr *)NLMSG_DATA(resp);
> > + if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> > + goto malformed_resp;
> > +
> > + switch (-err->error) {
>
> Please remove the unary "-", since it's not used here.
Ok.
>
> > + case 0:
> > + break;
>
> That "break;" should be indented.
Ok.
>
> > +
> > + default:
> > + virReportSystemError(-err->error,
> > +
_("error dumping
%d interface"),
> > +
ifindex);
> > + rc = -1;
> > + }
> > + break;
>
> Same here. Indent.
>
> > +
> > + case GENL_ID_CTRL:
> > + case NLMSG_DONE:
> > + if (nlmsg_parse(resp, sizeof(struct
ifinfomsg),
> > +
tb, IFLA_MAX, ifla_policy)) {
> > + goto malformed_resp;
> > + }
> > + break;
>
> And here. Indent the "break".
>
> > +
> > + default:
> > + goto malformed_resp;
> > + }
> > +
> > + if (rc != 0)
> > + VIR_FREE(*recvbuf);
> > +
> > + return rc;
> > +
> > +malformed_resp:
> > + macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("malformed
netlink response message"));
> > + VIR_FREE(*recvbuf);
> > + return -1;
> > +
> > +buffer_too_small:
> > + macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("internal
buffer is too small"));
> > + return -1;
> > +}
> ...
> > +static int
> > +doPortProfileOpSetLink(bool multicast,
> > +
int ifindex,
> > +
const char *profileId,
> > +
struct ifla_port_vsi *portVsi,
> > +
const unsigned char *instanceId,
> > +
const unsigned char *hostUUID,
> > +
int32_t vf,
> > +
uint8_t op)
> > +{
> > + int rc = 0;
> > + char nlmsgbuf[256];
> > + struct nlmsghdr *nlm = (struct nlmsghdr *)nlmsgbuf,
*resp;
> > + struct nlmsgerr *err;
> > + char rtattbuf[64];
>
> Can you use a symbolic constant instead of "64" ?
Had a previous patch adding that but it was abandoned.
Will re-add this.
>
> > + struct rtattr *rta, *vfports, *vfport;
> > + struct ifinfomsg ifinfo = {
> > + .ifi_family = AF_UNSPEC,
> > + .ifi_index = ifindex,
> > + };
> > + char *recvbuf = NULL;
> > + int recvbuflen = 0;
> > +
> > + memset(&nlmsgbuf, 0, sizeof(nlmsgbuf));
> > +
> > + nlInit(nlm, NLM_F_REQUEST, RTM_SETLINK);
> > +
> > + if (!nlAppend(nlm, sizeof(nlmsgbuf), &ifinfo,
sizeof(ifinfo)))
> > + goto buffer_too_small;
> > +
> > + if (vf == PORT_SELF_VF) {
> > + rta = rtattrCreate(rtattbuf, sizeof(rtattbuf),
> IFLA_PORT_SELF, NULL, 0);
> > + } else {
> > + rta = rtattrCreate(rtattbuf, sizeof(rtattbuf),
> IFLA_VF_PORTS, NULL, 0);
> > + if (!rta)
> > + goto buffer_too_small;
> > +
> > + if (!(vfports = nlAppend(nlm, sizeof(nlmsgbuf),
> > +
rtattbuf, rta->rta_len)))
> > + goto buffer_too_small;
> > +
> > + /* beging nesting vfports */
>
> Typo.
> s/beging/begin/
Ok.
>
> > + rta = rtattrCreate(rtattbuf, sizeof(rtattbuf),
> IFLA_VF_PORT, NULL, 0);
> > + }
> > +
> > + if (!rta)
> > + goto buffer_too_small;
> > +
> > + if (!(vfport = nlAppend(nlm, sizeof(nlmsgbuf),
rtattbuf,
> rta->rta_len)))
> > + goto buffer_too_small;
> > +
> > + if (profileId) {
> > + rta = rtattrCreate(rtattbuf, sizeof(rtattbuf),
IFLA_PORT_PROFILE,
> > +
profileId, strlen(profileId) + 1);
> > + if (!rta)
> > + goto buffer_too_small;
> > +
> > + if (!nlAppend(nlm, sizeof(nlmsgbuf),
rtattbuf, rta->rta_len))
> > + goto buffer_too_small;
> > + }
> > +
> > + if (portVsi) {
> > + rta = rtattrCreate(rtattbuf, sizeof(rtattbuf),
IFLA_PORT_VSI_TYPE,
> > +
portVsi, sizeof(*portVsi));
> > + if (!rta)
> > + goto buffer_too_small;
> > +
> > + if (!nlAppend(nlm, sizeof(nlmsgbuf),
rtattbuf, rta->rta_len))
> > + goto buffer_too_small;
> > + }
> > +
> > + if (instanceId) {
> > + rta = rtattrCreate(rtattbuf, sizeof(rtattbuf),
> IFLA_PORT_INSTANCE_UUID,
> > +
instanceId, VIR_UUID_BUFLEN);
> > + if (!rta)
> > + goto buffer_too_small;
> > +
> > + if (!nlAppend(nlm, sizeof(nlmsgbuf),
rtattbuf, rta->rta_len))
> > + goto buffer_too_small;
> > + }
> > +
> > + if (hostUUID) {
> > + rta = rtattrCreate(rtattbuf, sizeof(rtattbuf),
IFLA_PORT_HOST_UUID,
> > +
hostUUID, VIR_UUID_BUFLEN);
> > + if (!rta)
> > + goto buffer_too_small;
> > +
> > + if (!nlAppend(nlm, sizeof(nlmsgbuf),
rtattbuf, rta->rta_len))
> > + goto buffer_too_small;
> > + }
> > +
> > + if (vf != PORT_SELF_VF) {
> > + rta = rtattrCreate(rtattbuf, sizeof(rtattbuf),
IFLA_PORT_VF,
> > +
&vf, sizeof(vf));
> > + if (!rta)
> > + goto buffer_too_small;
> > +
> > + if (!nlAppend(nlm, sizeof(nlmsgbuf),
rtattbuf, rta->rta_len))
> > + goto buffer_too_small;
> > + }
> > +
> > + rta = rtattrCreate(rtattbuf, sizeof(rtattbuf),
IFLA_PORT_REQUEST,
> > +
&op, sizeof(op));
> > + if (!rta)
> > + goto buffer_too_small;
> > +
> > + if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf,
rta->rta_len))
> > + goto buffer_too_small;
>
> There is too much duplication in the above 6 clauses.
> Can you factor out a tiny helper function to make this more
> readable/maintainable?
>
> At the very least, combine the two
> "if...goto buffer_too_small" stmts into one:
>
> if (!rta || !nlAppend(nlm,
sizeof(nlmsgbuf), rtattbuf,
> rta->rta_len))
Alright. Will do. Though this may also tough existing
code.
> goto buffer_too_small;
> > +
> > + /* end nesting of vport */
> > + vfport->rta_len = (char *)nlm + nlm->nlmsg_len
- (char *)vfport;
> > +
> > + if (vf != PORT_SELF_VF) {
> > + /* end nesting of vfports */
> > + vfports->rta_len = (char *)nlm
+ nlm->nlmsg_len - (char *)vfports;
> > + }
> > +
> > + if (!multicast) {
> > + if (nlComm(nlm, &recvbuf, &recvbuflen)
< 0)
> > + return -1;
> > + } else {
> > + if (nlCommWaitSuccess(nlm, RTMGRP_LINK,
&recvbuf, &recvbuflen,
> > +
5 * MICROSEC_PER_SEC) < 0)
> > + return -1;
> > + }
> > +
> > + if (recvbuflen < NLMSG_LENGTH(0) || recvbuf
== NULL)
> > + goto malformed_resp;
> > +
> > + resp = (struct nlmsghdr *)recvbuf;
> > +
> > + switch (resp->nlmsg_type) {
> > + case NLMSG_ERROR:
> > + err = (struct nlmsgerr *)NLMSG_DATA(resp);
> > + if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
> > + goto malformed_resp;
> >
> > + switch (-err->error) {
> > + case 0:
> > + break;
>
> Indent the break:
> case 0:
> break;
>
> Actually, since there's only one action here,
> just drop the switch in favor of a simple "if" stmt.
> (same with the other switch (-err->error) above)
> That makes the resulting code shorter by 4 lines per switch stmt.
Ok. Had done that to be consistent with other code.
>
> if (err->error) {
> virReportSystemError(-err->error,
> _("error
during virtual port configuration of ifindex %d"),
> ifindex);
> rc = -1;
> }
>
> > + default:
> > + virReportSystemError(-err->error,
> > + _("error
during virtual port configuration of ifindex %d"),
> > + ifindex);
> > + rc = -1;
> > + }
> > + break;
>
> Indent.
Ok.
>
> > +
> > + case NLMSG_DONE:
> > + break;
>
> Indent.
Ok.
>
> > +
> > + default:
> > + goto malformed_resp;
> > + }
> > +
> > + VIR_FREE(recvbuf);
> > +
> > + return rc;
> > +
> > +malformed_resp:
> > + macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("malformed
netlink response message"));
> > + VIR_FREE(recvbuf);
> > + return -1;
> > +
> > +buffer_too_small:
> > + macvtapError(VIR_ERR_INTERNAL_ERROR, "%s",
> > + _("internal
buffer is too small"));
> > + return -1;
> > +}
> > +
> > +
> > +static int
> > +doPortProfileOpCommon(bool multicast,
> > +
int ifindex,
> > +
const char *profileId,
> > +
struct ifla_port_vsi *portVsi,
> > +
const unsigned char *instanceId,
> > +
const unsigned char *hostUUID,
> > +
int32_t vf,
> > +
uint8_t op)
> > +{
> > + int rc;
> > + char *recvbuf = NULL;
> > + struct nlattr *tb[IFLA_MAX + 1];
> > + int repeats = 80;
>
> This can (hence should) be unsigned.
> This number is obviously related to the 125000-microsecond
> sleep below. Consider adding a comment saying up to how long
> we should wait for a result.
I'll add some math to this.
>
> > + uint16_t status = 0;
> > +
> > + rc = doPortProfileOpSetLink(multicast,
> > +
ifindex,
> > +
profileId,
> > +
portVsi,
> > +
instanceId,
> > +
hostUUID,
> > +
vf,
> > +
op);
> > +
> > + if (rc != 0) {
> > + macvtapError(VIR_ERR_INTERNAL_ERROR,
"%s",
> > +
_("sending of PortProfileRequest failed."));
> > + return rc;
> > + }
> > +
> > + if (!multicast) {
> > + while (--repeats) {
> > + rc = link_dump(ifindex,
tb, &recvbuf);
> > + if (rc)
> > + goto
err_exit;
> > + rc = getPortProfileStatus(tb,
vf, &status);
> > + if (rc == 0) {
>
> What if getPortProfileStatus returns something other than 0?
> Currently, when that happens, this code just ignores it (failure?)
> sleeps and retries.
> Doesn't failure to get status deserve an error just
> as much as when we do get status that indicates a failure?
Yes, it should. Should only happen, though, if we
are all of a sudden on a wrong kernel...
>
> > + if (status
== PORT_PROFILE_RESPONSE_SUCCESS ||
> > +
status == PORT_VDP_RESPONSE_SUCCESS) {
> > +
break;
> > + } else
if (status == PORT_PROFILE_RESPONSE_INPROGRESS) {
> > +
// keep trying...
> > + } else
{
> > +
virReportSystemError(EINVAL,
> > +
_("error %d during port-profile setlink
> on ifindex %d"),
> > +
status, ifindex);
> > +
rc = 1;
> > +
break;
> > + }
> > + }
> > + usleep(125000);
>
> Magic constant?
> How did you determine this number?
> How often are we expected to hit it?
> Please add a comment.
This *may* be something I suggested -- to wait for
1/8 sec. Not sure whether this is
too much, or not fast enough, but more like a shot
into the 'blue'.
>
> > + VIR_FREE(recvbuf);
> > + }
> > + }
> > +
> > + if (status == PORT_PROFILE_RESPONSE_INPROGRESS)
{
> > + macvtapError(VIR_ERR_INTERNAL_ERROR,
"%s",
> > +
_("port-profile setlink timed out"));
> > + rc = -ETIMEDOUT;
> > + }
> > +
> > +err_exit:
> > + VIR_FREE(recvbuf);
> > +
> > + return rc;
> > +}
> > +
> > +# endif /* IFLA_PORT_MAX */
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
I'll post v7 addressing the comments.
Stefan