[PATCH] virsh: Provide completer for virtualization types

natto1784 posted 1 patch 2 years, 1 month ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20220325225143.1533634-1-natto@weirdnatto.in
.gitignore                   |  4 ++++
tools/virsh-completer-host.c | 12 ++++++++++++
tools/virsh-completer-host.h |  5 +++++
tools/virsh-host.c           |  3 +++
4 files changed, 24 insertions(+)
[PATCH] virsh: Provide completer for virtualization types
Posted by natto1784 2 years, 1 month ago
Related: https://gitlab.com/libvirt/libvirt/-/issues/9
Signed-off-by: natto1784 <natto@weirdnatto.in>
---
 .gitignore                   |  4 ++++
 tools/virsh-completer-host.c | 12 ++++++++++++
 tools/virsh-completer-host.h |  5 +++++
 tools/virsh-host.c           |  3 +++
 4 files changed, 24 insertions(+)

diff --git a/.gitignore b/.gitignore
index 4695391..62012f4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -23,4 +23,8 @@ tags
 
 # clangd related ignores
 .clangd
+.cache/clangd
 compile_commands.json
+
+# ccls cache
+.ccls-cache
diff --git a/tools/virsh-completer-host.c b/tools/virsh-completer-host.c
index 40cb687..e481a73 100644
--- a/tools/virsh-completer-host.c
+++ b/tools/virsh-completer-host.c
@@ -27,6 +27,7 @@
 #include "virxml.h"
 #include "virutil.h"
 #include "virsh-host.h"
+#include "conf/domain_conf.h"
 
 static char *
 virshPagesizeNodeToString(xmlNodePtr node)
@@ -180,3 +181,14 @@ virshNodeSuspendTargetCompleter(vshControl *ctl G_GNUC_UNUSED,
     return virshEnumComplete(VIR_NODE_SUSPEND_TARGET_LAST,
                              virshNodeSuspendTargetTypeToString);
 }
+
+char **
+virshVirtTypeCompleter(vshControl *ctl G_GNUC_UNUSED,
+                       const vshCmd *cmd G_GNUC_UNUSED,
+                       unsigned int flags)
+{
+    virCheckFlags(0, NULL);
+
+    return virshEnumComplete(VIR_DOMAIN_VIRT_LAST,
+                             virDomainVirtTypeToString);
+}
diff --git a/tools/virsh-completer-host.h b/tools/virsh-completer-host.h
index e71ccff..372ac14 100644
--- a/tools/virsh-completer-host.h
+++ b/tools/virsh-completer-host.h
@@ -41,3 +41,8 @@ char **
 virshNodeSuspendTargetCompleter(vshControl *ctl,
                                 const vshCmd *cmd,
                                 unsigned int flags);
+
+char **
+virshVirtTypeCompleter(vshControl *ctl,
+                       const vshCmd *cmd,
+                       unsigned int flags);
diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index 2e3cbc3..b28f29f 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -77,6 +77,7 @@ static const vshCmdInfo info_domcapabilities[] = {
 static const vshCmdOptDef opts_domcapabilities[] = {
     {.name = "virttype",
      .type = VSH_OT_STRING,
+     .completer = virshVirtTypeCompleter,
      .help = N_("virtualization type (/domain/@type)"),
     },
     {.name = "emulatorbin",
@@ -1577,6 +1578,7 @@ static const vshCmdOptDef opts_hypervisor_cpu_compare[] = {
     VIRSH_COMMON_OPT_FILE(N_("file containing an XML CPU description")),
     {.name = "virttype",
      .type = VSH_OT_STRING,
+     .completer = virshVirtTypeCompleter,
      .help = N_("virtualization type (/domain/@type)"),
     },
     {.name = "emulator",
@@ -1686,6 +1688,7 @@ static const vshCmdOptDef opts_hypervisor_cpu_baseline[] = {
     VIRSH_COMMON_OPT_FILE(N_("file containing XML CPU descriptions")),
     {.name = "virttype",
      .type = VSH_OT_STRING,
+     .completer = virshVirtTypeCompleter,
      .help = N_("virtualization type (/domain/@type)"),
     },
     {.name = "emulator",
-- 
2.35.1
Re: [PATCH] virsh: Provide completer for virtualization types
Posted by Michal Prívozník 2 years, 1 month ago
On 3/25/22 23:51, natto1784 wrote:
> Related: https://gitlab.com/libvirt/libvirt/-/issues/9
> Signed-off-by: natto1784 <natto@weirdnatto.in>
> ---
>  .gitignore                   |  4 ++++
>  tools/virsh-completer-host.c | 12 ++++++++++++
>  tools/virsh-completer-host.h |  5 +++++
>  tools/virsh-host.c           |  3 +++
>  4 files changed, 24 insertions(+)
> 
> diff --git a/.gitignore b/.gitignore
> index 4695391..62012f4 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -23,4 +23,8 @@ tags
>  
>  # clangd related ignores
>  .clangd
> +.cache/clangd
>  compile_commands.json
> +
> +# ccls cache
> +.ccls-cache

I believe this is the first time I see anybody opening libvirt sources
in Visual Studio :-)

Anyway, this hunk is unrelated to the rest of the patch and therefore
should be a separate commit.

> diff --git a/tools/virsh-completer-host.c b/tools/virsh-completer-host.c
> index 40cb687..e481a73 100644
> --- a/tools/virsh-completer-host.c
> +++ b/tools/virsh-completer-host.c
> @@ -27,6 +27,7 @@
>  #include "virxml.h"
>  #include "virutil.h"
>  #include "virsh-host.h"
> +#include "conf/domain_conf.h"
>  
>  static char *
>  virshPagesizeNodeToString(xmlNodePtr node)
> @@ -180,3 +181,14 @@ virshNodeSuspendTargetCompleter(vshControl *ctl G_GNUC_UNUSED,
>      return virshEnumComplete(VIR_NODE_SUSPEND_TARGET_LAST,
>                               virshNodeSuspendTargetTypeToString);
>  }
> +
> +char **
> +virshVirtTypeCompleter(vshControl *ctl G_GNUC_UNUSED,
> +                       const vshCmd *cmd G_GNUC_UNUSED,
> +                       unsigned int flags)
> +{
> +    virCheckFlags(0, NULL);
> +
> +    return virshEnumComplete(VIR_DOMAIN_VIRT_LAST,
> +                             virDomainVirtTypeToString);
> +}
> diff --git a/tools/virsh-completer-host.h b/tools/virsh-completer-host.h
> index e71ccff..372ac14 100644
> --- a/tools/virsh-completer-host.h
> +++ b/tools/virsh-completer-host.h
> @@ -41,3 +41,8 @@ char **
>  virshNodeSuspendTargetCompleter(vshControl *ctl,
>                                  const vshCmd *cmd,
>                                  unsigned int flags);
> +
> +char **
> +virshVirtTypeCompleter(vshControl *ctl,
> +                       const vshCmd *cmd,
> +                       unsigned int flags);
> diff --git a/tools/virsh-host.c b/tools/virsh-host.c
> index 2e3cbc3..b28f29f 100644
> --- a/tools/virsh-host.c
> +++ b/tools/virsh-host.c
> @@ -77,6 +77,7 @@ static const vshCmdInfo info_domcapabilities[] = {
>  static const vshCmdOptDef opts_domcapabilities[] = {
>      {.name = "virttype",
>       .type = VSH_OT_STRING,
> +     .completer = virshVirtTypeCompleter,
>       .help = N_("virtualization type (/domain/@type)"),
>      },
>      {.name = "emulatorbin",
> @@ -1577,6 +1578,7 @@ static const vshCmdOptDef opts_hypervisor_cpu_compare[] = {
>      VIRSH_COMMON_OPT_FILE(N_("file containing an XML CPU description")),
>      {.name = "virttype",
>       .type = VSH_OT_STRING,
> +     .completer = virshVirtTypeCompleter,
>       .help = N_("virtualization type (/domain/@type)"),
>      },
>      {.name = "emulator",
> @@ -1686,6 +1688,7 @@ static const vshCmdOptDef opts_hypervisor_cpu_baseline[] = {
>      VIRSH_COMMON_OPT_FILE(N_("file containing XML CPU descriptions")),
>      {.name = "virttype",
>       .type = VSH_OT_STRING,
> +     .completer = virshVirtTypeCompleter,
>       .help = N_("virtualization type (/domain/@type)"),
>      },
>      {.name = "emulator",

This part looks okay. Could you split this one into two patches then?

Michal
Re: [PATCH] virsh: Provide completer for virtualization types
Posted by Erik Skultety 2 years ago
On Tue, Mar 29, 2022 at 08:40:29AM +0200, Michal Prívozník wrote:
> On 3/25/22 23:51, natto1784 wrote:
> > Related: https://gitlab.com/libvirt/libvirt/-/issues/9
> > Signed-off-by: natto1784 <natto@weirdnatto.in>
> > ---
> >  .gitignore                   |  4 ++++
> >  tools/virsh-completer-host.c | 12 ++++++++++++
> >  tools/virsh-completer-host.h |  5 +++++
> >  tools/virsh-host.c           |  3 +++
> >  4 files changed, 24 insertions(+)
> > 
> > diff --git a/.gitignore b/.gitignore
> > index 4695391..62012f4 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -23,4 +23,8 @@ tags
> >  
> >  # clangd related ignores
> >  .clangd
> > +.cache/clangd
> >  compile_commands.json
> > +
> > +# ccls cache
> > +.ccls-cache

Neither of ^these are actually related in any way to the project itself or the
toolchain adopted by the project, on the contrary both relate to user's
working environment. Therefore these should not be placed in the project's
gitignore and instead be put in your own (global) one.
I didn't even know we had clangd bits in there which should have never been the
case IMO (I spotted this patch by an accident).
A bit of background: I was told the same thing in the virt-manager project and
ever since I'm using my own gitignore for everything that does not strictly
relate the toolchain used by the project.

Regards,
Erik

Re: [PATCH] virsh: Provide completer for virtualization types
Posted by Michal Prívozník 2 years ago
On 4/1/22 09:02, Erik Skultety wrote:
> On Tue, Mar 29, 2022 at 08:40:29AM +0200, Michal Prívozník wrote:
>> On 3/25/22 23:51, natto1784 wrote:
>>> Related: https://gitlab.com/libvirt/libvirt/-/issues/9
>>> Signed-off-by: natto1784 <natto@weirdnatto.in>
>>> ---
>>>  .gitignore                   |  4 ++++
>>>  tools/virsh-completer-host.c | 12 ++++++++++++
>>>  tools/virsh-completer-host.h |  5 +++++
>>>  tools/virsh-host.c           |  3 +++
>>>  4 files changed, 24 insertions(+)
>>>
>>> diff --git a/.gitignore b/.gitignore
>>> index 4695391..62012f4 100644
>>> --- a/.gitignore
>>> +++ b/.gitignore
>>> @@ -23,4 +23,8 @@ tags
>>>  
>>>  # clangd related ignores
>>>  .clangd
>>> +.cache/clangd
>>>  compile_commands.json
>>> +
>>> +# ccls cache
>>> +.ccls-cache
> 
> Neither of ^these are actually related in any way to the project itself or the
> toolchain adopted by the project, on the contrary both relate to user's
> working environment. Therefore these should not be placed in the project's
> gitignore and instead be put in your own (global) one.
> I didn't even know we had clangd bits in there which should have never been the
> case IMO (I spotted this patch by an accident).
> A bit of background: I was told the same thing in the virt-manager project and
> ever since I'm using my own gitignore for everything that does not strictly
> relate the toolchain used by the project.

I don't disagree, but we already have records for
vim/emacs/clang/python(?). I believe the same argument can be made about
those files too.

BTW: why do we have __pycache__/ in there? Isn't that a relic from the
old times, when python bindings were living in the same repo?

Michal

Re: [PATCH] virsh: Provide completer for virtualization types
Posted by Erik Skultety 2 years ago
On Fri, Apr 01, 2022 at 09:24:01AM +0200, Michal Prívozník wrote:
> On 4/1/22 09:02, Erik Skultety wrote:
> > On Tue, Mar 29, 2022 at 08:40:29AM +0200, Michal Prívozník wrote:
> >> On 3/25/22 23:51, natto1784 wrote:
> >>> Related: https://gitlab.com/libvirt/libvirt/-/issues/9
> >>> Signed-off-by: natto1784 <natto@weirdnatto.in>
> >>> ---
> >>>  .gitignore                   |  4 ++++
> >>>  tools/virsh-completer-host.c | 12 ++++++++++++
> >>>  tools/virsh-completer-host.h |  5 +++++
> >>>  tools/virsh-host.c           |  3 +++
> >>>  4 files changed, 24 insertions(+)
> >>>
> >>> diff --git a/.gitignore b/.gitignore
> >>> index 4695391..62012f4 100644
> >>> --- a/.gitignore
> >>> +++ b/.gitignore
> >>> @@ -23,4 +23,8 @@ tags
> >>>  
> >>>  # clangd related ignores
> >>>  .clangd
> >>> +.cache/clangd
> >>>  compile_commands.json
> >>> +
> >>> +# ccls cache
> >>> +.ccls-cache
> > 
> > Neither of ^these are actually related in any way to the project itself or the
> > toolchain adopted by the project, on the contrary both relate to user's
> > working environment. Therefore these should not be placed in the project's
> > gitignore and instead be put in your own (global) one.
> > I didn't even know we had clangd bits in there which should have never been the
> > case IMO (I spotted this patch by an accident).
> > A bit of background: I was told the same thing in the virt-manager project and
> > ever since I'm using my own gitignore for everything that does not strictly
> > relate the toolchain used by the project.
> 
> I don't disagree, but we already have records for
> vim/emacs/clang/python(?). I believe the same argument can be made about
> those files too.

I admit I wasn't very clear in what I wrote, so correcting it now - yes, you
are right, I noticed it along with the clangd bits and I never noticed them
until now as I never really had a need to keep an eye on the file using my
local gitignores :). However, that doesn't mean it should become a precedent.
If anything we should remove those too, but at the same time I don't feel a
strong justification for such a change, so instead we should politely refuse
any further changes like this one (strictly non-related to the project) to the
gitignore from now on.

> 
> BTW: why do we have __pycache__/ in there? Isn't that a relic from the
> old times, when python bindings were living in the same repo?

Probably, but the same applies...

Erik