Re: [libvirt] [PATCHv2 2/4] util: add virNetDevVlanType
On 08/14/2012 06:55 PM, Eric Blake wrote:
On 08/14/2012 01:15 AM, Laine Stump wrote:
> To allow for the possibility of vlan "trunks", which have more than
> one vlan tag associated with them, we need a vlan struct. Since it
> will be used by multiple files in src/util, src/conf, src/network, and
> src/qemu, it must be defined in src/util. Unfortunately there isn't
> currently a common file for simple netdev data definitions, so I
> created a new file.
> +int
> +virNetDevVlanEqual(const virNetDevVlanPtr a, const virNetDevVlanPtr b)
> +{
> + int ii;
> +
> + if (!(a || b))
> + return true;
> + if (!a || !b)
> + return false;
> +
> + if (a->trunk != b->trunk ||
> + a->nTags != b->nTags) {
> + return false;
> + }
> +
> + for (ii = 0; ii < a->nTags; ii++) {
> + if (a->tag[ii] != b->tag[ii])
> + return false;
I think this requires the user to list tags in identical order. Should
two virNetDevVlanPtr be considered equal if the only difference is the
order in which tags are listed, or is there some other reason why an
unsorted list would lose significance if sorted?
Really it was just lack of deeper thought, and the fact that my only
planned used for this function is to decide if things have changed
enough to, for example, disallow a "live" change of network config
(currently it's not used at all).
I've changed the algorithm to the following. Unfortunately I again
wasn't thinking, and changed it during a git rebase, so I don't have a
diff handy. Should I resend the entire patch, or is seeing this changed
function enough for an ACK?
(Note that the following would return true if one array was {1,1,2,2,2}
and the other was {2,1,2,1,1} (i.e. if both have all the same members,
but with differing counts of each). However, it makes no sense for a
vlan trunk to list the same tag more than once anyway, so I guess
effectively they *would* be the same.)
int
virNetDevVlanEqual(const virNetDevVlanPtr a, const virNetDevVlanPtr b)
{
int ai, bi;
if (!(a || b))
return true;
if (!a || !b)
return false;
if (a->trunk != b->trunk ||
a->nTags != b->nTags) {
return false;
}
for (ai = 0; ai < a->nTags; ai++) {
for (bi = 0; bi < b->nTags; bi++) {
if (a->tag[ai] == b->tag[bi])
break;
}
if (bi >= b->nTags) {
/* no matches for a->tag[ai] anywhere in b->tag */
return false;
}
}
return true;
}
> +int
> +virNetDevVlanCopy(virNetDevVlanPtr dst, const virNetDevVlanPtr src)
> +{
> + if (!src || src->nTags == 0)
> + return 0;
> +
> + if (VIR_ALLOC_N(dst->tag, src->nTags) < 0) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + dst->trunk = src->trunk;
> + dst->nTags = src->nTags;
> + memmove(dst->tag, src->tag, src->nTags * sizeof(*src->tag));
src and dst better not overlap; therefore, memcpy() is a better fit
Okay, changed that.