Re: [libvirt] [PATCH 13/19] virsh-domain: Introduce 2 macros for domain options 'interface' and 'mac'
On 2020-11-02 19:40, Michal Privoznik wrote:
On 11/2/20 9:26 AM, Lin Ma wrote:
> The macro VIRSH_DOMAIN_OPT_INTERFACE for domain option '--interface',
> The macro VIRSH_DOMAIN_OPT_MAC for domain option '--mac'.
>
> Signed-off-by: Lin Ma <lma(a)suse.com>
> ---
> tools/virsh-domain.h | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/tools/virsh-domain.h b/tools/virsh-domain.h
> index 0d59c579d4..ac05f983f9 100644
> --- a/tools/virsh-domain.h
> +++ b/tools/virsh-domain.h
> @@ -39,3 +39,21 @@ typedef enum {
> VIR_ENUM_DECL(virshDomainHostnameSource);
> extern const vshCmdDef domManagementCmds[];
> +
> +#define VIRSH_DOMAIN_OPT_INTERFACE(_helpstr, oflags, cflags) \
> + {.name = "interface", \
> + .type = VSH_OT_STRING, \
> + .flags = oflags, \
> + .help = _helpstr, \
> + .completer = virshDomainInterfaceCompleter, \
> + .completer_flags = cflags, \
> + }
> +
> +#define VIRSH_DOMAIN_OPT_MAC(_helpstr, oflags) \
> + {.name = "mac", \
> + .type = VSH_OT_STRING, \
> + .flags = oflags, \
> + .help = _helpstr, \
> + .completer = virshDomainInterfaceCompleter, \
> + .completer_flags = VIRSH_DOMAIN_INTERFACE_COMPLETER_MAC, \
> + }
>
So, if we had two distinct completers, as I suggested, then this would
look a bit different and both macros could accept cflags. We could
then print MAC addresses only of running/inactive/all interfaces,
which could be helpful.
The idea about two distinct completers, It's about virsh-interface, But
in
virsh-domain, Here is only one interface completer
virshDomainInterfaceCompleter
and it can handle name/mac well with a cflags.
IMO, The question is that we have two form of parameters: "interface"
and "mac"
in virsh-domain. That's why I created two macros.
If we replace '--mac' by '--interface' in virsh-domain, Then things go
easy:
With cflags's help, We don't need 2 macros any more, But obviously it
changed
the existing virsh cmd usage, Probably It's not acceptable. So I created
macro
VIRSH_DOMAIN_OPT_MAC only for the parameter '--mac', The '--mac' should
accept
mac address string only, Does a cflags really make sense to this macro?
The discussion about macros perhaps impact the review of the patchset
v2, so I
removed them in patchset v2, If we need them, I'll post them later.
I'm stopping my review here. I think you get the gist of my
review.
Looking forward to v2.
Thank you so much for your nice review! I just posted v2 to libvirt ml.
Thank again,
LLin