On 05/15/2018 01:43 PM, Daniel P. Berrangé wrote:
> $ virsh nwfilter-binding-list
> Port Dev Filter
> ------------------------------------------------------------------
> vnet0 clean-traffic
> vnet1 clean-traffic
I know this is text cut-n-paste, but this does seem off a bit for
alignment between data and column header...
>
> $ virsh nwfilter-binding-dumpxml vnet1
> <filterbinding>
> <owner>
> <name>f25arm7</name>
> <uuid>12ac8b8c-4f23-4248-ae42-fdcd50c400fd</uuid>
> </owner>
> <portdev name='vnet1'/>
> <mac address='52:54:00:9d:81:b1'/>
> <filterref filter='clean-traffic'>
> <parameter name='MAC' value='52:54:00:9d:81:b1'/>
> </filterref>
> </filterbinding>
>
> Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
> ---
> tools/virsh-completer.c | 45 ++++++
> tools/virsh-completer.h | 4 +
> tools/virsh-nwfilter.c | 318 ++++++++++++++++++++++++++++++++++++++++
> tools/virsh-nwfilter.h | 8 +
> 4 files changed, 375 insertions(+)
>
virsh.pod?
There's probably enough here to ask for a respin, but it shouldn't
impact the subsequent patches...
[...]
> diff --git a/tools/virsh-nwfilter.c b/tools/virsh-nwfilter.c
> index 06a002dffd..881afc5dda 100644
> --- a/tools/virsh-nwfilter.c
> +++ b/tools/virsh-nwfilter.c
> @@ -443,6 +443,300 @@ cmdNWFilterEdit(vshControl *ctl, const vshCmd *cmd)
> return ret;
> }
>
These all have only 1 blank line between functions.
> +virNWFilterBindingPtr
> +virshCommandOptNWFilterBindingBy(vshControl *ctl, const vshCmd *cmd,
> + const char **name, unsigned int flags)
1 line per argument
> +{
> + virNWFilterBindingPtr binding = NULL;
> + const char *n = NULL;
> + const char *optname = "binding";
> + virshControlPtr priv = ctl->privData;
> +
[...]
> +
> +static virshNWFilterBindingListPtr
> +virshNWFilterBindingListCollect(vshControl *ctl,
> + unsigned int flags)
> +{
> + virshNWFilterBindingListPtr list = vshMalloc(ctl, sizeof(*list));
> + size_t i;
> + int ret;
> + bool success = false;
> + size_t deleted = 0;
> + int nbindings = 0;
> + char **names = NULL;
deleted, nbindings, and names are not used.
> + virshControlPtr priv = ctl->privData;
> +
> + /* try the list with flags support (0.10.2 and later) */
Probably can strike the comment.
> + if ((ret = virConnectListAllNWFilterBindings(priv->conn,
> + &list->bindings,
> + flags)) < 0) {
> + /* there was an error during the call */
> + vshError(ctl, "%s", _("Failed to list network filter bindings"));
> + goto cleanup;
> + }
> +
> + list->nbindings = ret;
> +
> + /* sort the list */
So the default will be to sort by portdev name then? Should it be an
option?. If it's the default, we just need to be sure to document it
that way. Naturally someone will want it sorted by something else...
> + if (list->bindings && list->nbindings)
nbindings > 1
> + qsort(list->bindings, list->nbindings,
> + sizeof(*list->bindings), virshNWFilterBindingSorter);
> +
> + /* truncate the list for not found filter objects */
> + if (deleted)
> + VIR_SHRINK_N(list->bindings, list->nbindings, deleted);
> +
> + success = true;
> +
> + cleanup:
> + for (i = 0; nbindings != -1 && i < nbindings; i++)
> + VIR_FREE(names[i]);
> + VIR_FREE(names);
I think the above is replaced by the next hunk...
> +
> + if (!success) {
> + virshNWFilterBindingListFree(list);
> + list = NULL;
> + }
> +
> + return list;
> +}
> +
> +/*
> + * "nwfilter-binding-list" command
> + */
> +static const vshCmdInfo info_nwfilter_binding_list[] = {
> + {.name = "help",
> + .data = N_("list network filter bindings")
> + },
> + {.name = "desc",
> + .data = N_("Returns list of network filter bindings.")
> + },
> + {.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_nwfilter_binding_list[] = {
> + {.name = NULL}
> +};
> +
> +static bool
> +cmdNWFilterBindingList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED)
> +{
> + size_t i;
> + virshNWFilterBindingListPtr list = NULL;
> +
> + if (!(list = virshNWFilterBindingListCollect(ctl, 0)))
> + return false;
> +
> + vshPrintExtra(ctl, " %-36s %-20s \n", _("Port Dev"), _("Filter"));
> + vshPrintExtra(ctl, "---------------------------------"
> + "---------------------------------\n");
> +
> + for (i = 0; i < list->nbindings; i++) {
> + virNWFilterBindingPtr binding = list->bindings[i];
> +
> + vshPrint(ctl, " %-20s %-20s\n",
You give 20s here, but 36s above - perhaps why it appeared off in the
commit message...
> + virNWFilterBindingGetPortDev(binding),
> + virNWFilterBindingGetFilterName(binding));
> + }
> +
> + virshNWFilterBindingListFree(list);
> + return true;
> +}
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
