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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list