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

Re: [libvirt] [PATCH] virsh: Improve editing



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 :-)


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