[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] virsh: Fix a regression of detaching device

On 2011年12月20日 02:51, Eric Blake wrote:
On 12/19/2011 11:38 AM, Eric Blake wrote:
On 12/19/2011 04:01 AM, Osier Yang wrote:
Since for node<address>, the values for attrs "domain", "bus", "slot",
"function" are all converted to heximal when parsing the XML. However
commit ea7182c29 tries to examine if the device represented by the XML
from user exists in the domain XML earlier before the XML parsing, and
thus if the user use different base to represent the device address, e.g.
   <address type='pci' domain='0' bus='0' slot='7'/>

vshCompleteXMLFromDomain won't find the device, and quit with error
"no such device" even if the device does exist.
  tools/virsh.c |   41 +++++++++++++++++++++++++++++++++++------
  1 files changed, 35 insertions(+), 6 deletions(-)

Hmm.  I agree that we have a problem, but I think this patch is taking
the wrong approach to fixing it.  By special casing which pieces of XML
should be parsed as an integer and compared by integer value rather than
by string equality, we effectively end up having to add more and more
exceptions every time someone else points out another field that needs
to be treated as an integer.

Yes, I'd say commit ea7182c29 might be not a quite good idea for
we do somethings special internally, just like for the address
conversion, it's not quite good idea to do things like early
comparison in virsh, but since it's already pushed, the better
way is to find some way to work around.

It would be a lot better to read the user's input, and normalize it into
a single form, then compare the normalized forms; rather than this
patch's attempt to normalize individual fields during the comparison.
But I'm not sure how we would go about doing that, short of reusing our
XML parsing code from domain_conf.c which does the normalization by
flattening XML into a consistent in-memory representation, followed by
the code that re-generates XML from that in-memory representation.

Maybe we need a new API, something like:

char *virConnectNormalizeXML(virConnectPtr conn, const char *xml,
unsigned int flags)

that takes incoming xml, passes it through the internal parser, and
spits back out the resulting xml (or diagnoses the xml as invalid by
returning NULL), all without modifying any existing domain, network,
storage pool, or other object managed by xml.

Sounds great idea, but the only question in my mind is how useful
the API will be. Seems it will be useful only for the app which
does things like virsh, or some one wants to see the dry-run XML

And it inspires me perhaps it will be great if we support dry-run
XML not only for the XML pieces, but also for the whole domain,
network, storage, interface, ... configs too?


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]