[libvirt] [PATCH] virsh: Improve editing

[Laine Stump]

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