On 05/13/2011 10:23 AM, Cole Robinson wrote:
On 05/13/2011 08:57 AM, Jiri Denemark wrote:
On Fri, May 13, 2011 at 12:34:25 +0200, Michal Privoznik wrote:
When users (pool-/net-)edit and they make a mistake, temporary file
and thus all changes are gone. Better way is to let them decide if
they want to get back to edit and correct what's wrong.
However, this is suitable only in interactive mode.
---
tools/virsh.c | 42 ++++++++++++++++++++++++++++++++++++++++--
1 files changed, 40 insertions(+), 2 deletions(-)
I like the idea (not sure if that's because I suggested it :-P) but, I have a
few comments. The patch doesn't touch iface-edit, which is not generated from
cmdEdit, though the right fix might turn out to be generating cmdIfaceEdit
together with cmdPoolEdit and cmdNetworkEdit.
I think we should just drop the 'edit' generation. It's a one-off that
doesn't fit with the rest of the virsh code. I'm sure most of the shared
functionality could be broken out into helper functions.
Ah, yes. I'd forgotten all about that. Looking back at the code to
remember why I didn't make cmdInterfaceEdit another generated file like
cmdPoolEdit and cmdNetEdit, I come up with the following: I didn't like
the the idea of generated code that was creating by scraping a piece out
of an existing C file. It all felt very hack-ey, and I didn't want to
perpetuate that. (I should have gone back later and done something in
one direction or the other to eliminate this hackiness, but as I said, I
immediately forgot all about it until I was reminded now, nearly two
years later).
I would probably have no complaint about it if the original source used
for the generation was not just something dredged out of an existing C
file, but was instead kept separately, and was truly a template rather
than an actual function that was also compiled as-is (ie cmdEdit was
also generated, and the strings being replaced were clearly marked in
the original as such).
However, I think I prefer Cole's idea of putting the shared
functionality into helper functions, or possibly a single function that
took pointers to a few object-specific functions as arguments, so it
could be called like this:
ret = cmdEdit(ctl, cmd, vshCommandOptInterface,
virInterfaceGetXMLDesc,
virInterfaceGetName, virInterfaceDefineXML,
virInterfaceFree,
VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE; );
(there again the (easily solved) problem is that all of the
vir*DefineXML take a flags argument, except for virDomainDefineXML. And
of course there's the problem that the functions for each object type do
not have exactly the same prototype, since the object being pointed to
is different in each case. But this is C, so pesky type-checking
problems can always be eliminated :-)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list