Re: [libvirt] [PATCH] virsh: new net-update command

>> + {"xml", VSH_OT_BOOL, 0, N_("xml is specified directly on commandline")}, >> + {"file", VSH_OT_BOOL, 0, N_("file containing xml is specified on commandline")}, >> + {"xmldata", VSH_OT_DATA, VSH_OFLAG_REQ, >> + N_("complete xml element (or name of file containing xml) to add/modify, " >> + "or to be matched for search")}, > Interesting choice to make --xml and --file be boolean flags, and > '[--xmldata] data' be the string that becomes either the file name or > the xml content. I might have done just two optional VSH_OT_DATA > arguments for --xml and --file and then manually checked that exactly > one of the two was supplied, instead of using three arguments. But what > you did works, so no need to change it. That's how I originally did it, but I didn't like being required to always say "--file xyz.xml" (while the existing commands that take a filename don't require that - the filename option can be positionally determined). Dan gave me the idea of having the (optionally unnamed) string option, along with two booleans. Of course, "--file" is kind of pointless, since that's the default behavior anyway.

I'm kind of on the fence between the two methods right now - whether to be more compatible with existing command syntax, or have fewer options. I realize that one problem of having the two booleans is that if somebody wants to specify the args in a different order, they would need to use the name of the string option, and it would end up looking something like this: net-update add --file --xmldata myfile.xml --section ip-dhcp-host --parent-index 4 --live

(Likely nobody would ever knowingly choose to do it that way, but...) So, the two possibilities are: 1) two string options, --file and --xml, so the command would look like one of these: net-update add ip-dhcp-host --file xyz.xml --live net-update add ip-dhcp-host --xml "<host mac='x:x:x:x:x:x' ip='1.2.3.4'/>" 2) two booleans and a string which is mandatory, but can be unnamed if in the right position: net-update add ip-dhcp-host xyz.xml --live net-update add ip-dhcp-host --xml "<host mac='x:x:x:x:x:x' ip='1.2.3.4'/>"

Any votes for one or the other?

>> + if (virFileReadAll(xmldata, VSH_MAX_XML_FILE, &xmldataFromFile) < 0) >> + return false; >> + /* NB: the original xmldata is freed by the vshCommand >> + * infrastructure, so it's safe to lose its pointer here. > Misleading comment; rather, xmldata is a 'const char *' and doesn't need > to be freed. Well, it points to data that is in an arg that's parsed/allocated by vshCommandParse, and later freed by vshCommandFree, so both are correct statements. I'll change it to something that makes us both happy :-)