[libvirt] [PATCH 6/7] list: Use virConnectListAllNetworks in virsh
Osier Yang
jyang at redhat.com
Tue Sep 11 10:45:06 UTC 2012
On 2012年09月11日 00:29, Laine Stump wrote:
> On 09/04/2012 11:55 AM, Osier Yang wrote:
>> tools/virsh-network.c:
>> * vshNetworkSorter to sort networks by name
>>
>> * vshNetworkListFree to free the network objects list.
>>
>> * vshNetworkListCollect to collect the network objects, trying
>> to use new API first, fall back to older APIs if it's not supported.
>>
>> * New options --persistent, --transient, --autostart, --no-autostart,
>> for net-list, and new field 'Persistent' for its output.
>>
>> tools/virsh.pod:
>> * Add documents for the new options.
>> ---
>> tools/virsh-network.c | 352 ++++++++++++++++++++++++++++++++++++------------
>> tools/virsh.pod | 12 ++-
>> 2 files changed, 275 insertions(+), 89 deletions(-)
>>
>> diff --git a/tools/virsh-network.c b/tools/virsh-network.c
>> index db204af..f6623ff 100644
>> --- a/tools/virsh-network.c
>> +++ b/tools/virsh-network.c
>> @@ -36,6 +36,7 @@
>> #include "memory.h"
>> #include "util.h"
>> #include "xml.h"
>> +#include "conf/network_conf.h"
>
>
> I've gotta say that (as discussed before) I don't like including
> something from the conf directory here. I think it's the case that this
> is only being done so that virsh can provide the new functionality even
> when only the old API is available, but I think it should be done in a
> self-contained manner, at least partly because people will look to virsh
> as an example of how to use the libvirt API. I guess I'm okay with
> leaving it this way for now, but I think it really needs to be cleaned up.
>
>
>>
>> virNetworkPtr
>> vshCommandOptNetworkBy(vshControl *ctl, const vshCmd *cmd,
>> @@ -342,6 +343,225 @@ cmdNetworkInfo(vshControl *ctl, const vshCmd *cmd)
>> return true;
>> }
>>
>> +static int
>> +vshNetworkSorter(const void *a, const void *b)
>> +{
>> + virNetworkPtr *na = (virNetworkPtr *) a;
>> + virNetworkPtr *nb = (virNetworkPtr *) b;
>> +
>> + if (*na&& !*nb)
>> + return -1;
>> +
>> + if (!*na)
>> + return *nb != NULL;
>> +
>> + return vshStrcasecmp(virNetworkGetName(*na),
>> + virNetworkGetName(*nb));
>
>
> This use of strcasecmp mmade me go back to verify that case *is*
> significant in the rest of the network_conf code (so, for example, you
> can have both a network named "ABC" and one named "Abc"). It's still
> useful to have strcasecmp here, though, as it makes the ordering ignore
> case, which is a "good thing"(tm).
>
>
>> +}
>> +
>> +struct vshNetworkList {
>> + virNetworkPtr *nets;
>> + size_t nnets;
>> +};
>> +typedef struct vshNetworkList *vshNetworkListPtr;
>
> The fact that you're doing this leaves me wondering if maybe
> virNetworkList should be a part of the API (along with a
> "virNetworkListFree()" as I've previously mentioned). After all, the
> fact that this first example of using the new API is doing this is a
> reasonably good indicator that future users will be copying the same code.
>
>
>> +
>> +static void
>> +vshNetworkListFree(vshNetworkListPtr list)
>> +{
>> + int i;
>> +
>> + if (list&& list->nnets) {
>> + for (i = 0; i< list->nnets; i++) {
>> + if (list->nets[i])
>> + virNetworkFree(list->nets[i]);
>> + }
>> + VIR_FREE(list->nets);
>> + }
>> + VIR_FREE(list);
>> +}
>> +
>> +static vshNetworkListPtr
>> +vshNetworkListCollect(vshControl *ctl,
>> + unsigned int flags)
>> +{
>> + vshNetworkListPtr list = vshMalloc(ctl, sizeof(*list));
>> + int i;
>> + int ret;
>> + char **names = NULL;
>> + virNetworkPtr net;
>> + bool success = false;
>> + size_t deleted = 0;
>> + int persistent;
>> + int autostart;
>> + int nActiveNets = 0;
>> + int nInactiveNets = 0;
>> + int nAllNets = 0;
>> +
>> + /* try the list with flags support (0.10.0 and later) */
>> + if ((ret = virConnectListAllNetworks(ctl->conn,
>> +&list->nets,
>> + flags))>= 0) {
>> + list->nnets = ret;
>> + goto finished;
>> + }
>> +
>> + /* check if the command is actually supported */
>> + if (last_error&& last_error->code == VIR_ERR_NO_SUPPORT) {
>> + vshResetLibvirtError();
>> + goto fallback;
>> + }
>> +
>> + if (last_error&& last_error->code == VIR_ERR_INVALID_ARG) {
>> + /* try the new API again but mask non-guaranteed flags */
>> + unsigned int newflags = flags& (VIR_CONNECT_LIST_NETWORKS_ACTIVE |
>> + VIR_CONNECT_LIST_NETWORKS_INACTIVE);
>> +
>> + vshResetLibvirtError();
>> + if ((ret = virConnectListAllNetworks(ctl->conn,&list->nets,
>> + newflags))>= 0) {
>> + list->nnets = ret;
>> + goto filter;
>> + }
>> + }
>> +
>> + /* there was an error during the first or second call */
>> + vshError(ctl, "%s", _("Failed to list networks"));
>> + goto cleanup;
>> +
>> +
>> +fallback:
>> + /* fall back to old method (0.9.13 and older) */
>
>
> Well, okay, I'll mention this incorrect version number, because it's a
> different number than the others.
>
>
>> + vshResetLibvirtError();
>
>
> As was pointed out in the nwfilter patches, this 2nd
> vshResetLibvirtError() is redundant; you only need it in one place or
> the other, but not both.
>
>
>> +
>> + /* Get the number of active networks */
>> + if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) ||
>> + MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE)) {
>> + if ((nActiveNets = virConnectNumOfNetworks(ctl->conn))< 0) {
>> + vshError(ctl, "%s", _("Failed to get the number of active networks"));
>> + goto cleanup;
>> + }
>> + }
>> +
>> + /* Get the number of inactive networks */
>> + if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) ||
>> + MATCH(VIR_CONNECT_LIST_NETWORKS_INACTIVE)) {
>> + if ((nInactiveNets = virConnectNumOfDefinedNetworks(ctl->conn))< 0) {
>> + vshError(ctl, "%s", _("Failed to get the number of inactive networks"));
>> + goto cleanup;
>> + }
>> + }
>> +
>> + nAllNets = nActiveNets + nInactiveNets;
>> +
>> + if (nAllNets == 0)
>> + return list;
>> +
>> + names = vshMalloc(ctl, sizeof(char *) * nAllNets);
>> +
>> + /* Retrieve a list of active network names */
>> + if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) ||
>> + MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE)) {
>> + if (virConnectListNetworks(ctl->conn,
>> + names, nActiveNets)< 0) {
>> + vshError(ctl, "%s", _("Failed to list active networks"));
>> + goto cleanup;
>> + }
>> + }
>> +
>> + /* Add the inactive networks to the end of the name list */
>> + if (!MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_ACTIVE) ||
>> + MATCH(VIR_CONNECT_LIST_NETWORKS_ACTIVE)) {
>> + if (virConnectListDefinedNetworks(ctl->conn,
>> +&names[nActiveNets],
>> + nInactiveNets)< 0) {
>> + vshError(ctl, "%s", _("Failed to list inactive networks"));
>> + goto cleanup;
>> + }
>> + }
>> +
>> + list->nets = vshMalloc(ctl, sizeof(virNetworkPtr) * (nAllNets));
>> + list->nnets = 0;
>> +
>> + /* get active networks */
>> + for (i = 0; i< nActiveNets; i++) {
>> + if (!(net = virNetworkLookupByName(ctl->conn, names[i])))
>> + continue;
>> + list->nets[list->nnets++] = net;
>> + }
>> +
>> + /* get inactive networks */
>> + for (i = 0; i< nInactiveNets; i++) {
>> + if (!(net = virNetworkLookupByName(ctl->conn, names[i])))
>> + continue;
>> + list->nets[list->nnets++] = net;
>> + }
>> +
>> + /* truncate networks that weren't found */
>> + deleted = nAllNets - list->nnets;
>> +
>> +filter:
>> + /* filter list the list if the list was acquired by fallback means */
>> + for (i = 0; i< list->nnets; i++) {
>> + net = list->nets[i];
>> +
>> + /* persistence filter */
>> + if (MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_PERSISTENT)) {
>> + if ((persistent = virNetworkIsPersistent(net))< 0) {
>> + vshError(ctl, "%s", _("Failed to get network persistence info"));
>> + goto cleanup;
>> + }
>> +
>> + if (!((MATCH(VIR_CONNECT_LIST_NETWORKS_PERSISTENT)&& persistent) ||
>> + (MATCH(VIR_CONNECT_LIST_NETWORKS_TRANSIENT)&& !persistent)))
>> + goto remove_entry;
>> + }
>> +
>> + /* autostart filter */
>> + if (MATCH(VIR_CONNECT_LIST_NETWORKS_FILTERS_AUTOSTART)) {
>> + if (virNetworkGetAutostart(net,&autostart)< 0) {
>> + vshError(ctl, "%s", _("Failed to get network autostart state"));
>> + goto cleanup;
>> + }
>> +
>> + if (!((MATCH(VIR_CONNECT_LIST_NETWORKS_AUTOSTART)&& autostart) ||
>> + (MATCH(VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART)&& !autostart)))
>> + goto remove_entry;
>> + }
>> + /* the pool matched all filters, it may stay */
>> + continue;
>> +
>> +remove_entry:
>> + /* the pool has to be removed as it failed one of the filters */
>> + virNetworkFree(list->nets[i]);
>> + list->nets[i] = NULL;
>> + deleted++;
>> + }
>> +
>> +finished:
>> + /* sort the list */
>> + if (list->nets&& list->nnets)
>> + qsort(list->nets, list->nnets,
>> + sizeof(*list->nets), vshNetworkSorter);
>> +
>> + /* truncate the list if filter simulation deleted entries */
>> + if (deleted)
>> + VIR_SHRINK_N(list->nets, list->nnets, deleted);
>> +
>> + success = true;
>> +
>> +cleanup:
>> + for (i = 0; i< nAllNets; i++)
>> + VIR_FREE(names[i]);
>> + VIR_FREE(names);
>> +
>> + if (!success) {
>> + vshNetworkListFree(list);
>> + list = NULL;
>> + }
>> +
>> + return list;
>> +}
>> +
>> /*
>> * "net-list" command
>> */
>> @@ -354,114 +574,70 @@ static const vshCmdInfo info_network_list[] = {
>> static const vshCmdOptDef opts_network_list[] = {
>> {"inactive", VSH_OT_BOOL, 0, N_("list inactive networks")},
>> {"all", VSH_OT_BOOL, 0, N_("list inactive& active networks")},
>> + {"persistent", VSH_OT_BOOL, 0, N_("list persistent networks")},
>> + {"transient", VSH_OT_BOOL, 0, N_("list transient networks")},
>> + {"autostart", VSH_OT_BOOL, 0, N_("list networks with autostart enabled")},
>> + {"no-autostart", VSH_OT_BOOL, 0, N_("list networks with autostart disabled")},
>> {NULL, 0, 0, NULL}
>> };
>>
>> static bool
>> cmdNetworkList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
>> {
>> + vshNetworkListPtr list = NULL;
>> + int i;
>> bool inactive = vshCommandOptBool(cmd, "inactive");
>> bool all = vshCommandOptBool(cmd, "all");
>> - bool active = !inactive || all;
>> - int maxactive = 0, maxinactive = 0, i;
>> - char **activeNames = NULL, **inactiveNames = NULL;
>> - inactive |= all;
>> -
>> - if (active) {
>> - maxactive = virConnectNumOfNetworks(ctl->conn);
>> - if (maxactive< 0) {
>> - vshError(ctl, "%s", _("Failed to list active networks"));
>> - return false;
>> - }
>> - if (maxactive) {
>> - activeNames = vshMalloc(ctl, sizeof(char *) * maxactive);
>> -
>> - if ((maxactive = virConnectListNetworks(ctl->conn, activeNames,
>> - maxactive))< 0) {
>> - vshError(ctl, "%s", _("Failed to list active networks"));
>> - VIR_FREE(activeNames);
>> - return false;
>> - }
>> + bool persistent = vshCommandOptBool(cmd, "persistent");
>> + bool transient = vshCommandOptBool(cmd, "transient");
>> + bool autostart = vshCommandOptBool(cmd, "autostart");
>> + bool no_autostart = vshCommandOptBool(cmd, "no-autostart");
>> + unsigned int flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE;
>>
>> - qsort(&activeNames[0], maxactive, sizeof(char *), vshNameSorter);
>> - }
>> - }
>> - if (inactive) {
>> - maxinactive = virConnectNumOfDefinedNetworks(ctl->conn);
>> - if (maxinactive< 0) {
>> - vshError(ctl, "%s", _("Failed to list inactive networks"));
>> - VIR_FREE(activeNames);
>> - return false;
>> - }
>> - if (maxinactive) {
>> - inactiveNames = vshMalloc(ctl, sizeof(char *) * maxinactive);
>> -
>> - if ((maxinactive =
>> - virConnectListDefinedNetworks(ctl->conn, inactiveNames,
>> - maxinactive))< 0) {
>> - vshError(ctl, "%s", _("Failed to list inactive networks"));
>> - VIR_FREE(activeNames);
>> - VIR_FREE(inactiveNames);
>> - return false;
>> - }
>> + if (inactive)
>> + flags = VIR_CONNECT_LIST_NETWORKS_INACTIVE;
>>
>> - qsort(&inactiveNames[0], maxinactive, sizeof(char*), vshNameSorter);
>> - }
>> - }
>> - vshPrintExtra(ctl, "%-20s %-10s %s\n", _("Name"), _("State"),
>> - _("Autostart"));
>> - vshPrintExtra(ctl, "-----------------------------------------\n");
>> + if (all)
>> + flags = VIR_CONNECT_LIST_NETWORKS_ACTIVE |
>> + VIR_CONNECT_LIST_NETWORKS_INACTIVE;
>>
>> - for (i = 0; i< maxactive; i++) {
>> - virNetworkPtr network =
>> - virNetworkLookupByName(ctl->conn, activeNames[i]);
>> - const char *autostartStr;
>> - int autostart = 0;
>> + if (persistent)
>> + flags |= VIR_CONNECT_LIST_NETWORKS_PERSISTENT;
>>
>> - /* this kind of work with networks is not atomic operation */
>> - if (!network) {
>> - VIR_FREE(activeNames[i]);
>> - continue;
>> - }
>> + if (transient)
>> + flags |= VIR_CONNECT_LIST_NETWORKS_TRANSIENT;
>>
>> - if (virNetworkGetAutostart(network,&autostart)< 0)
>> - autostartStr = _("no autostart");
>> - else
>> - autostartStr = autostart ? _("yes") : _("no");
>> + if (autostart)
>> + flags |= VIR_CONNECT_LIST_NETWORKS_AUTOSTART;
>>
>> - vshPrint(ctl, "%-20s %-10s %-10s\n",
>> - virNetworkGetName(network),
>> - _("active"),
>> - autostartStr);
>> - virNetworkFree(network);
>> - VIR_FREE(activeNames[i]);
>> - }
>> - for (i = 0; i< maxinactive; i++) {
>> - virNetworkPtr network = virNetworkLookupByName(ctl->conn, inactiveNames[i]);
>> - const char *autostartStr;
>> - int autostart = 0;
>> + if (no_autostart)
>> + flags |= VIR_CONNECT_LIST_NETWORKS_NO_AUTOSTART;
>>
>> - /* this kind of work with networks is not atomic operation */
>> - if (!network) {
>> - VIR_FREE(inactiveNames[i]);
>> - continue;
>> - }
>> + if (!(list = vshNetworkListCollect(ctl, flags)))
>> + return false;
>> +
>> + vshPrintExtra(ctl, "%-20s %-10s %-13s %s\n", _("Name"), _("State"),
>> + _("Autostart"), _("Persistent"));
>> + vshPrintExtra(ctl, "--------------------------------------------------\n");
>
>
> Do you think we need to worry about adding a new column onto the output
> having an adverse affect on existing scripts that parse the output?
>
>
>>
>> - if (virNetworkGetAutostart(network,&autostart)< 0)
>> + for (i = 0; i< list->nnets; i++) {
>> + virNetworkPtr network = list->nets[i];
>> + const char *autostartStr;
>> + int is_autostart = 0;
>> +
>> + if (virNetworkGetAutostart(network,&is_autostart)< 0)
>> autostartStr = _("no autostart");
>> else
>> - autostartStr = autostart ? _("yes") : _("no");
>> -
>> - vshPrint(ctl, "%-20s %-10s %-10s\n",
>> - inactiveNames[i],
>> - _("inactive"),
>> - autostartStr);
>> + autostartStr = is_autostart ? _("yes") : _("no");
>>
>> - virNetworkFree(network);
>> - VIR_FREE(inactiveNames[i]);
>> + vshPrint(ctl, "%-20s %-10s %-13s %s\n",
>> + virNetworkGetName(network),
>> + virNetworkIsActive(network) ? _("active") : _("inactive"),
>> + autostartStr,
>> + virNetworkIsPersistent(network) ? _("yes") : _("no"));
>> }
>> - VIR_FREE(activeNames);
>> - VIR_FREE(inactiveNames);
>> +
>> + vshNetworkListFree(list);
>> return true;
>> }
>>
>> diff --git a/tools/virsh.pod b/tools/virsh.pod
>> index 9aeb47e..c806335 100644
>> --- a/tools/virsh.pod
>> +++ b/tools/virsh.pod
>> @@ -1933,10 +1933,20 @@ variables, and defaults to C<vi>.
>> Returns basic information about the I<network> object.
>>
>> =item B<net-list> [I<--inactive> | I<--all>]
>> + [I<--persistent>] [<--transient>]
>> + [I<--autostart>] [<--no-autostart>]
>>
>> Returns the list of active networks, if I<--all> is specified this will also
>> include defined but inactive networks, if I<--inactive> is specified only the
>> -inactive ones will be listed.
>> +inactive ones will be listed. You may also want to filter the returned networks
>> +by I<--persistent> to list the persitent ones, I<--transient> to list the
>> +transient ones, I<--autostart> to list the ones with autostart enabled, and
>> +I<--no-autostart> to list the ones with autostart disabled.
>> +
>> +NOTE: When talking to older servers, this command is forced to use a series of
>> +API calls with an inherent race, where a pool might not be listed or might appear
>> +more than once if it changed state between calls while the list was being
>> +collected. Newer servers do not have this problem.
>>
>> =item B<net-name> I<network-UUID>
>>
>
> In the interest of having the API in the release, I would be okay with
> pushing as-is, but would rather have the questions above cleared up first.
Hope I get the questions cleared up, :-)
I pushed the set, but I'm ready to work if problems/questions
are still existed.
Thanks for the reviewing!
Osier
More information about the libvir-list
mailing list