On 07/16/2012 05:14 AM, Stefan Berger wrote:
> Introduce new members in the virMacAddr 'class'
> - virMacAddrSet: set virMacAddr from a virMacAddr
> - virMacAddrSetRaw: setting virMacAddr from raw 6 byte MAC address buffer
> - virMacAddrGetRaw: writing virMacAddr into raw 6 byte MAC address buffer
> - virMacAddrCmp: comparing two virMacAddr
> - virMacAddrCmpRaw: comparing a virMacAddr with a raw 6 byte MAC address buffer
>
> then replace raw MAC addresses by replacing
>
> - 'unsigned char *' with virMacAddrPtr
> - 'unsigned char ... [VIR_MAC_BUFLEN]' with virMacAddr
>
> and introduce usage of above functions where necessary.
>
You didn't mention what changed since v1, so I ended up reviewing this
entire email even if I could have focused on particular sections.
> + * Copy src to dst
> + */
> +void
> +virMacAddrSet(virMacAddrPtr dst, const virMacAddrPtr src)
> + * Set the MAC address to the given value
> + */
> +void
> +virMacAddrSetRaw(virMacAddrPtr dst, const unsigned char src[VIR_MAC_BUFLEN])
> + * Copies the MAC address into raw memory
> + */
> +void
> +virMacAddrGetRaw(virMacAddrPtr src, unsigned char dst[VIR_MAC_BUFLEN])
> +{
> + memcpy(dst, src->addr, VIR_MAC_BUFLEN);
Given the other two functions being 'dst' as the first argument, this
almost feels backwards (here, you are setting 'dst' as the second
argument based on reading the first). On the other hand, since most
everything else in this file is a function whose first argument is a
virMacAddrPtr, maybe it makes sense with this ordering. Also, our Parse
function uses this ordering (source is a string as first argument,
destination is the address it was parsed into).
> int virMacAddrCompare(const char *mac1, const char *mac2);
> -void virMacAddrFormat(const unsigned char *addr,
> +int virMacAddrCmp(const virMacAddrPtr mac1, const virMacAddrPtr mac2);
> +int virMacAddrCmpRaw(const virMacAddrPtr mac1,
> + const unsigned char s[VIR_MAC_BUFLEN]);
As long as we are touching this header, should we add some
ATTRIBUTE_NONNULL markings?
> @@ -589,13 +589,13 @@ virNetDevMacVLanVPortProfileCallback(uns
> VIR_DEBUG("IFLA_VF_MAC = %2x:%2x:%2x:%2x:%2x:%2x",
> m[0], m[1], m[2], m[3], m[4], m[5]);
>
> - if (memcmp(calld->macaddress, m, VIR_MAC_BUFLEN))
> + if (virMacAddrCmpRaw(&calld->macaddress, mac->mac))
> {
> /* Repeat the same check for a broadcast mac */
> int i;
>
> for (i = 0;i < VIR_MAC_BUFLEN; i++) {
> - if (calld->macaddress[i] != 0xff) {
> + if (calld->macaddress.addr[i] != 0xff) {
As long as you are touching this area, s/;i/; i/ in the previous line.
> @@ -85,7 +86,7 @@ int virNetDevMacVLanRestartWithVPortProf
> ATTRIBUTE_NONNULL(4) ATTRIBUTE_RETURN_CHECK;
>
> int virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname,
> - const unsigned char *macaddress ,
> + const virMacAddrPtr macaddress ,
s/ ,/,/
> @@ -127,7 +121,7 @@ struct _nwItemDesc {
> virNWFilterVarAccessPtr varAccess;
> enum attrDatatype datatype;
> union {
> - nwMACAddress macaddr;
> + virMacAddr macaddr;
> virSocketAddr ipaddr;
> bool boolean;
> uint8_t u8;
Were these all intended to be aligned?
In general, I only found formatting nits; the first two issues I pointed
out aren't show-stoppers. Let's get this in now, since it touches a lot
of files, and is mostly mechanical.
ACK.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list