On 06/08/2012 12:06 PM, Shradha Shah wrote:
> Hello all,
>
> I have actually based these patches of v0.9.12.
>
> Top of tree libvirt had some errors due to which I was not able to use virsh and test these patches so I based them off the last release.
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.
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list