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

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120919/7f3bb84b/attachment-0001.sig>