On 09/19/2012 06:04 PM, Laine Stump wrote:
+ {"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 not strongly tied to either approach, so does anyone else want to chime in?
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
This example's missing '--network net' as well, but yes, it illustrates the point.
(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'/>"
Option 3 - have a single mandatory string argument, then treat it as XML if it begins with '<' and as a filename otherwise (and if you happen to have a file in the current directory starting with '<', then prefix it with './').
Any votes for one or the other?
Sorry I'm not being more decisive on this one.
+ 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 :-)
Maybe: xmldata is a const char* alias into memory managed by other variables, so it doesn't need to be freed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org