Re: [libvirt] [PATCH 0/5] Support forward mode='hostdev' and interface pools

On 06/08/2012 12:06 PM, Shradha Shah wrote: Do you still have the same problem with the latest from git? It will be really nice to have this functionality in - I've considered the type='hostdev' support incomplete without it. Some general comments beyond (or reinforcing) those I already made in responses to the individual patches: * You have no documentation (should be added to docs/formatnetwork.html.in and docs/formatdomain.html.in), which really needs to be in the same patch that enables the new functionality. * Likewise I didn't see any new test case (tests/networkxml2xml*). * In general, I think it's useful to arrange multiple patches like this: * One or more patches that re-factor existing code (without causing any functional change) to prepare for the new code. * A patch that adds the new XML parsing/formatting, updates the RNG, and adds xml2xml tests. * A patch that connects the new functionality to the backend (in this case, much of that is already done, since the "<interface type='hostdev'>" patches were written such that networkAllocationActualDevice is called at the appropriate place and the actualdevice object is queried when appropriate (obviously there will be bugs in this, since it couldn't be properly tested until now). You may find that it useful to split the 3rd item into 2 parts: 1) a patch that updates the network driver to properly utilize the new attributes/elements that are now in virNetworkDef (i.e. allowing the pf element to populate all the devs/addresses) and 2) A patch that fixes any latent bugs in the qemu driver utilizing the returned dev/address). I guess in the end there are 4 rules of thumb I try to think of when I'm putting together a patch series (others probably have their own set of personal rules): 1) try to make each patch as easy to understand as possible, while avoiding having a large number of related, yet trivial, patches. 2) unrelated changes (or changes that could be useful separately from the rest of the series, e.g. general purpose utility functions) should be in separate patches. 3) try to avoid code churn - if something will move, move it once; when adding new code, the patch that introduces it should introduce it where you want it to be at the end of the series. 4) above anything else, it is a requirement that make check (and other existing libvirt operation) work correctly at any step in applying the patches. Sometimes this will require temporarily putting in a small bit of stub code, omitting/modifying a test, or even combining two patches that you would rather have separated, but it is necessary in order for git bisect to be useful.