On 05/11/2012 04:07 AM, Michal Privoznik wrote:
> On 10.05.2012 19:10, Laine Stump wrote:
>> On 05/09/2012 09:36 AM, Michal Privoznik wrote:
>>> If users {net-,pool-,}edit but make a mistake in XML all changes
>>> are permanently lost. However, if virsh is running in interactive
>>> mode we can as user if he wants to re-edit the file and correct
>>> the mistakes.
>> This all reminds me that I so much disliked the idea of creating a .c
>> file by running sed against virsh.c and then #include'ing that .c file
>> into virsh.c that I instead made a separate function for
>> cmdInterfaceEdit rather than adding to it. I had thought that someone
>> refactored that long ago so that the main bit of code was a helper
>> function called by all (thus removing the sed trickery), but apparently
>> we only talked about it. (*adds a line to todo list*)
> Heh, thanks for pointing that out. I've completely forgotten about
> iface-edit. So my patch is incomplete. :(
> On the other hand, seems like sed script is doing its work well. I've
> taken look at generated code and it was good. Even when I've introduced
> this dom_edited variable, it was renamed to network_edited and
> pool_edited respectively in those generated functions. So honestly,
> unless we are doing some different magic in cmdInterfaceEdit (quick look
> doesn't say so) I think we can believe the sed script and make
> cmdInterfaceEdit being generated as well. The advantage is - if somebody
> (this time it's me) introduce any feature, fix a bug, all other *-edit
> commands benefit from it immediately.
I agree that the code should only be there once, but rather than
creating all the other versions with a strange sed script in the
makefile, I think the correct solution is the more usual/accepted method
of making a generic helper function that parameterizes everything that
is different between the different types of edit, with a short function
for each version that calls this helper function. (I have this bad
feeling that I once promised to do that, but then forgot and never made
good on my promise :-/)
(or alternately, doing it with a macro as Dan suggests. That makes for a
pretty long macro, though :-)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list