Re: [libvirt] [PATCH] Added capability to set and restore the MAC address of a NIC when using PASSTHROUGH mode with DIRECT type NICs. This is required to support SRIOV capable NICs with firmware implemented on-card switches.

On 06/10/2011 05:40 AM, Dirk Herrendoerfer wrote: ...see my earlier mail about the cover letter containing useful commit information. Also, your subject line is way long; typically the subject should be < 60 characters, then a blank line, then you can give detailed information about the patch. 'git shortlog -30' will give you an idea of why short subject lines are useful. Let's keep this list sorted, and stick with camelCase (that is, getMacaddr comes before openMacvtapTap). Also, you're not the first, but this file isn't very namespace clean; I'm wondering if we should first clean things up to use a virFunction prefix style for all the internally-exported functions in our macvtap wrapper file. s/devices's/device's/ trailing space; please make sure your patch passes 'make syntax-check' first. Style: we usually don't put space immediately inside the (): if (net->data.direct.mode == VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU) { Style - space after comma: old_macaddr[0], old_macaddr[1], ... Why so big, when you know that you have a fixed-width address that only takes sizeof("00:00:00:00:00:00") bytes? For that matter, since it appears that you are trying to create a file name, it would be more appropriate to use virAsprintf (to malloc the name, rather than doing it on the stack), so that the directory prefix can then be variable sized according to configure arguments. For example, see how daemon/libvirtd.c computes /var/run/libvirt if base_dir_prefix is "/var", but is flexible to being run by non-root ($HOME/.libvirt/run instead of /var/run). Here, and in several other places after a failed syscall, you want to report the value of errno (which is a positive value such as EACCES), not rc (which is often -1, and which makes no sense in virReportSystemError). Why the hardcoded 6? MAC addresses are fixed length; there's no need to pass that length as a magic number parameter. Same style issues as above. Same question about newmacname being stack-allocated. Another reason to use virAsprintf is to avoid potential stack overflow issues if net->data.direct.linkdev is really large. 'make syntax-check' will call you on this one. Use virFileResolveLink instead. I'm not necessarily a fan of sscanf (it misses out on some integer overflow issues), although I think this particular usage is probably safe enough. Odd indentation, and poor style with space before ;. Unnecessary mask and cast since oldmac is already the right type. You can simply use: oldmac[0] = old_macaddr[0]; for properly converting the int (necessary for sscanf) down to an unsigned char. Even nicer would be to use the POSIX-mandated sscanf("%hhx", &oldmac[0]) in the first place, if that were portable (unfortunately, it isn't portable, and gnulib intentionally doesn't provide a fixed sscanf implementation because sscanf suffers from silent overflow issues in general). It would probably be nicer to rewrite this whole thing to avoid sscanf, and parse directly into unsigned char without going through intermediate ints. In fact, it seems like the conversion from a string into a MAC address ought to be an already supported common routine... </me searches> Yep - virParseMacAddr looks like your friend. s/restults/results/ Another 'make syntax-check' violation. bcopy is dead. Use memcpy. I ran out of time to review the rest of this; the overall idea has merit, but there are a lot of changes needed in v2 before anything can be applied. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org