[libvirt] [PATCH 04/19] virsh: Rename virshInterfaceNameCompleter to virshInterfaceCompleter

Lin Ma posted 19 patches 5 years, 3 months ago
There is a newer version of this series
[libvirt] [PATCH 04/19] virsh: Rename virshInterfaceNameCompleter to virshInterfaceCompleter
Posted by Lin Ma 5 years, 3 months ago
Rename the function virshInterfaceNameCompleter to virshInterfaceCompleter
to make it a bit more generic.
The upcoming patch invokes it for mac completion.

Signed-off-by: Lin Ma <lma@suse.com>
---
 tools/virsh-completer-interface.c | 6 +++---
 tools/virsh-completer-interface.h | 6 +++---
 tools/virsh-interface.c           | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/virsh-completer-interface.c b/tools/virsh-completer-interface.c
index 8028db8746..777bb22b0b 100644
--- a/tools/virsh-completer-interface.c
+++ b/tools/virsh-completer-interface.c
@@ -26,9 +26,9 @@
 #include "virstring.h"
 
 char **
-virshInterfaceNameCompleter(vshControl *ctl,
-                            const vshCmd *cmd G_GNUC_UNUSED,
-                            unsigned int flags)
+virshInterfaceCompleter(vshControl *ctl,
+                        const vshCmd *cmd G_GNUC_UNUSED,
+                        unsigned int flags)
 {
     virshControlPtr priv = ctl->privData;
     virInterfacePtr *ifaces = NULL;
diff --git a/tools/virsh-completer-interface.h b/tools/virsh-completer-interface.h
index 893dee5a6b..2b382222d7 100644
--- a/tools/virsh-completer-interface.h
+++ b/tools/virsh-completer-interface.h
@@ -22,6 +22,6 @@
 
 #include "vsh.h"
 
-char ** virshInterfaceNameCompleter(vshControl *ctl,
-                                    const vshCmd *cmd,
-                                    unsigned int flags);
+char ** virshInterfaceCompleter(vshControl *ctl,
+                                const vshCmd *cmd,
+                                unsigned int flags);
diff --git a/tools/virsh-interface.c b/tools/virsh-interface.c
index 8cdbc6e85f..bd57648779 100644
--- a/tools/virsh-interface.c
+++ b/tools/virsh-interface.c
@@ -23,7 +23,7 @@
      .type = VSH_OT_DATA, \
      .flags = VSH_OFLAG_REQ, \
      .help = N_("interface name or MAC address"), \
-     .completer = virshInterfaceNameCompleter, \
+     .completer = virshInterfaceCompleter, \
      .completer_flags = cflags, \
     }
 
-- 
2.26.0


Re: [libvirt] [PATCH 04/19] virsh: Rename virshInterfaceNameCompleter to virshInterfaceCompleter
Posted by Michal Privoznik 5 years, 3 months ago
On 11/2/20 9:26 AM, Lin Ma wrote:
> Rename the function virshInterfaceNameCompleter to virshInterfaceCompleter
> to make it a bit more generic.
> The upcoming patch invokes it for mac completion.
> 
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
>   tools/virsh-completer-interface.c | 6 +++---
>   tools/virsh-completer-interface.h | 6 +++---
>   tools/virsh-interface.c           | 2 +-
>   3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/virsh-completer-interface.c b/tools/virsh-completer-interface.c
> index 8028db8746..777bb22b0b 100644
> --- a/tools/virsh-completer-interface.c
> +++ b/tools/virsh-completer-interface.c
> @@ -26,9 +26,9 @@
>   #include "virstring.h"
>   
>   char **
> -virshInterfaceNameCompleter(vshControl *ctl,
> -                            const vshCmd *cmd G_GNUC_UNUSED,
> -                            unsigned int flags)
> +virshInterfaceCompleter(vshControl *ctl,
> +                        const vshCmd *cmd G_GNUC_UNUSED,
> +                        unsigned int flags)

Looking into the future patches, I think we want a different approach. 
In one of future patches you will switch this to return MAC addresses 
too. Fair enough, points for code re-use. But The way it's implemented 
is confusing and in fact wrong (see my review to 08/19). How about:

1) Moving this body into a static helper, which would accept one 
argument more: callback to get get the desired string
2) This virshInterfaceNameCompleter() would then effectively end up a 
single line call of that callback with virInterfaceGetName() passed as 
the callback,
3) New virshInterfaceMac() would be defined with 
virInterfaceGetMACString() passed as the callback.

Michal