[PATCH 0/1] virsh: Add option '--no-pkttyagent'
As discussed in https://gitlab.com/libvirt/libvirt/-/issues/757. I'm not quite sure about using a long-only-option, since that's a first in virsh. But OTOH this option seems too insignificant to also use one of the precious short option characters for it. Please check and let me know what you think. Thanks! Jens Schmidt (1): virsh: Add option '--no-pkttyagent' NEWS.rst | 4 ++++ docs/manpages/virsh.rst | 8 ++++++++ tools/virsh.c | 13 ++++++++++++- tools/vsh.h | 1 + 4 files changed, 25 insertions(+), 1 deletion(-) -- 2.39.5
In scripts repeated execution of virsh can result in a lot of journal noise when pkttyagent gets registered with polkitd each time. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/757 --- NEWS.rst | 4 ++++ docs/manpages/virsh.rst | 8 ++++++++ tools/virsh.c | 13 ++++++++++++- tools/vsh.h | 1 + 4 files changed, 25 insertions(+), 1 deletion(-) diff --git a/NEWS.rst b/NEWS.rst index ad8910da4c..12ef87bfce 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,10 @@ v11.4.0 (unreleased) * **New features** + * virsh: Add option ``--no-pkttyagent`` + + That option suppresses registration of pkttyagent with polkitd. + * **Improvements** * **Bug fixes** diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index cef9959f16..f2fac0acad 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -140,6 +140,14 @@ Output elapsed time information for each command. +- ``--no-pkttyagent`` + +Do not register ``pkttyagent`` as authentication agent with the +polkit system daemon, even if ``virsh`` has been started from a +terminal. + + + - ``-v``, ``--version[=short]`` Ignore all other arguments, and prints the version of the libvirt library diff --git a/tools/virsh.c b/tools/virsh.c index 800e280c7a..6ae650ec89 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -21,6 +21,7 @@ #include <config.h> #include "virsh.h" +#include <assert.h> #include <unistd.h> #include <getopt.h> #include <sys/time.h> @@ -117,7 +118,8 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) keepalive_forced = true; } - if (virPolkitAgentAvailable() && + if (!ctl->no_pkttyagent && + virPolkitAgentAvailable() && !(pkagent = virPolkitAgentCreate())) virResetLastError(); @@ -446,6 +448,7 @@ virshUsage(void) " -q | --quiet quiet mode\n" " -r | --readonly connect readonly\n" " -t | --timing print timing information\n" + " --no-pkttyagent suppress registration of pkttyagent\n" " -v short version\n" " -V long version\n" " --version[=TYPE] version, TYPE is short or long (default short)\n" @@ -642,6 +645,7 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) { "quiet", no_argument, NULL, 'q' }, { "readonly", no_argument, NULL, 'r' }, { "timing", no_argument, NULL, 't' }, + { "no-pkttyagent", no_argument, NULL, 0 }, { "version", optional_argument, NULL, 'v' }, { NULL, 0, NULL, 0 }, }; @@ -739,6 +743,13 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) case 'V': virshShowVersion(ctl); exit(EXIT_SUCCESS); + case 0: + if (STREQ(opt[longindex].name, "no-pkttyagent")) { + ctl->no_pkttyagent = true; + break; + } else { + assert(false); + } case ':': for (i = 0; opt[i].name != NULL; i++) { if (opt[i].val == optopt) diff --git a/tools/vsh.h b/tools/vsh.h index 8b87c00ff4..3b75216e11 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -200,6 +200,7 @@ struct _vshControl { bool imode; /* interactive mode? */ bool quiet; /* quiet mode */ bool timing; /* print timing info? */ + bool no_pkttyagent; /* suppress registration of pkttyagent? */ int debug; /* print debug messages? */ char *logfile; /* log file name */ int log_fd; /* log file descriptor */ -- 2.39.5
On Mon, May 05, 2025 at 23:09:52 +0200, Jens Schmidt via Devel wrote: Since you domain uses DMARC protections, in order to properly honour your autorship you need to set the following git option: git config --global format.forceInBodyFrom true https://www.libvirt.org/submitting-patches.html#git-configuration That adds a "From: " line allowing git to use your proper name and address.
In scripts repeated execution of virsh can result in a lot of journal noise when pkttyagent gets registered with polkitd each time.
For patches we require that the author certifies that the patch conforms with the developer certificate of origin: https://www.libvirt.org/hacking.html#developer-certificate-of-origin
--- NEWS.rst | 4 ++++ docs/manpages/virsh.rst | 8 ++++++++ tools/virsh.c | 13 ++++++++++++- tools/vsh.h | 1 + 4 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/NEWS.rst b/NEWS.rst index ad8910da4c..12ef87bfce 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,10 @@ v11.4.0 (unreleased)
* **New features**
+ * virsh: Add option ``--no-pkttyagent`` + + That option suppresses registration of pkttyagent with polkitd.
We require that changes to 'NEWS.rst' are always a separate patch, with no other changes. (For possible backports; NEWS are not backported)
+ * **Improvements**
* **Bug fixes** diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index cef9959f16..f2fac0acad 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -140,6 +140,14 @@ Output elapsed time information for each command.
+- ``--no-pkttyagent`` + +Do not register ``pkttyagent`` as authentication agent with the +polkit system daemon, even if ``virsh`` has been started from a +terminal. + + + - ``-v``, ``--version[=short]``
Ignore all other arguments, and prints the version of the libvirt library diff --git a/tools/virsh.c b/tools/virsh.c index 800e280c7a..6ae650ec89 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -21,6 +21,7 @@ #include <config.h> #include "virsh.h"
+#include <assert.h> #include <unistd.h> #include <getopt.h> #include <sys/time.h> @@ -117,7 +118,8 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) keepalive_forced = true; }
- if (virPolkitAgentAvailable() && + if (!ctl->no_pkttyagent && + virPolkitAgentAvailable() && !(pkagent = virPolkitAgentCreate())) virResetLastError();
@@ -446,6 +448,7 @@ virshUsage(void) " -q | --quiet quiet mode\n" " -r | --readonly connect readonly\n" " -t | --timing print timing information\n" + " --no-pkttyagent suppress registration of pkttyagent\n" " -v short version\n" " -V long version\n" " --version[=TYPE] version, TYPE is short or long (default short)\n" @@ -642,6 +645,7 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) { "quiet", no_argument, NULL, 'q' }, { "readonly", no_argument, NULL, 'r' }, { "timing", no_argument, NULL, 't' }, + { "no-pkttyagent", no_argument, NULL, 0 }, { "version", optional_argument, NULL, 'v' }, { NULL, 0, NULL, 0 }, }; @@ -739,6 +743,13 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) case 'V': virshShowVersion(ctl); exit(EXIT_SUCCESS); + case 0: + if (STREQ(opt[longindex].name, "no-pkttyagent")) { + ctl->no_pkttyagent = true; + break; + } else { + assert(false);
As witnessed by the fact that you needed to include assert.h we don't use asserts in the code base. While this code path should not be possible to reach without doing a programming error we still should report an error using vshError as in all other cases.
+ } case ':': for (i = 0; opt[i].name != NULL; i++) { if (opt[i].val == optopt) diff --git a/tools/vsh.h b/tools/vsh.h index 8b87c00ff4..3b75216e11 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -200,6 +200,7 @@ struct _vshControl { bool imode; /* interactive mode? */ bool quiet; /* quiet mode */ bool timing; /* print timing info? */ + bool no_pkttyagent; /* suppress registration of pkttyagent? */ int debug; /* print debug messages? */ char *logfile; /* log file name */ int log_fd; /* log file descriptor */ -- 2.39.5
The rest looks good to me, so you can add: Reviewed-by: Peter Krempa <pkrempa@redhat.com> when sending the updated version.
Thanks for your review and comments! On 2025-05-06 10:24, Peter Krempa wrote:
As witnessed by the fact that you needed to include assert.h we don't use asserts in the code base.
While this code path should not be possible to reach without doing a programming error we still should report an error using vshError as in all other cases.
Do I need to worry about (needless) translation effort of that error message? Or should I just use a generic one like vshError(ctl, "%s", _("unknown error")); or, more specific, but probably less accurate, vshError(ctl, "%s", _("unknown option")); already used elsewhere in the code?
On 5/6/25 23:50, Jens Schmidt via Devel wrote:
Thanks for your review and comments!
On 2025-05-06 10:24, Peter Krempa wrote:
As witnessed by the fact that you needed to include assert.h we don't use asserts in the code base.
While this code path should not be possible to reach without doing a programming error we still should report an error using vshError as in all other cases.
Do I need to worry about (needless) translation effort of that error message? Or should I just use a generic one like
vshError(ctl, "%s", _("unknown error"));
or, more specific, but probably less accurate,
vshError(ctl, "%s", _("unknown option"));
already used elsewhere in the code?
Both strings are present in our code so translation should be automagic. Go with whatever you feel is more appropriate. Michal
This patchset should address all of your comments. Where "you" means Peter Krempa, I hope that the "--in-reply-to" does its job as I would expect it would do. Just to be sure, this is in reply to thread [1], namely to the message with ID <aBnHTttMLaNd3E3U@angien.pipo.sk>. Some minor nits I came across while implementing this, and you are totally free to a) ignore them, b) fix them yourself, or c) ask me to fix them in a separate patch: - You actually do use assertions, namely in vsh.c. This is where I got the idea from that using them in virsh.c, too, would be OK. (Well, I guess option c) from above isn't something I'd want to do for that nit...) - In docs/manpages/virsh.rst, you use plain paragraphs for the documentation of options --connect and --debug and bulleted items for all other options. Thanks for your support and for maintaining libvirt in general! [1]: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/Y5JHS... Jens Schmidt (2): virsh: Add option '--no-pkttyagent' NEWS: Mention new option '--no-pkttyagent' NEWS.rst | 4 ++++ docs/manpages/virsh.rst | 8 ++++++++ tools/virsh.c | 13 ++++++++++++- tools/vsh.h | 1 + 4 files changed, 25 insertions(+), 1 deletion(-) -- 2.39.5
From: Jens Schmidt <farblos@vodafonemail.de> In scripts repeated execution of virsh can result in a lot of journal noise when pkttyagent gets registered with polkitd each time. Closes: https://gitlab.com/libvirt/libvirt/-/issues/757 Signed-off-by: Jens Schmidt <farblos@vodafonemail.de> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- docs/manpages/virsh.rst | 8 ++++++++ tools/virsh.c | 13 ++++++++++++- tools/vsh.h | 1 + 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index 0a54f6deff..3a00778467 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -140,6 +140,14 @@ Output elapsed time information for each command. +- ``--no-pkttyagent`` + +Do not register ``pkttyagent`` as authentication agent with the +polkit system daemon, even if ``virsh`` has been started from a +terminal. + + + - ``-v``, ``--version[=short]`` Ignore all other arguments, and prints the version of the libvirt library diff --git a/tools/virsh.c b/tools/virsh.c index 800e280c7a..c893216637 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -117,7 +117,8 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) keepalive_forced = true; } - if (virPolkitAgentAvailable() && + if (!ctl->no_pkttyagent && + virPolkitAgentAvailable() && !(pkagent = virPolkitAgentCreate())) virResetLastError(); @@ -446,6 +447,7 @@ virshUsage(void) " -q | --quiet quiet mode\n" " -r | --readonly connect readonly\n" " -t | --timing print timing information\n" + " --no-pkttyagent suppress registration of pkttyagent\n" " -v short version\n" " -V long version\n" " --version[=TYPE] version, TYPE is short or long (default short)\n" @@ -642,6 +644,7 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) { "quiet", no_argument, NULL, 'q' }, { "readonly", no_argument, NULL, 'r' }, { "timing", no_argument, NULL, 't' }, + { "no-pkttyagent", no_argument, NULL, 0 }, { "version", optional_argument, NULL, 'v' }, { NULL, 0, NULL, 0 }, }; @@ -739,6 +742,14 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) case 'V': virshShowVersion(ctl); exit(EXIT_SUCCESS); + case 0: + if (STREQ(opt[longindex].name, "no-pkttyagent")) { + ctl->no_pkttyagent = true; + break; + } else { + vshError(ctl, "%s", _("unknown option")); + exit(EXIT_FAILURE); + } case ':': for (i = 0; opt[i].name != NULL; i++) { if (opt[i].val == optopt) diff --git a/tools/vsh.h b/tools/vsh.h index 8b87c00ff4..3b75216e11 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -200,6 +200,7 @@ struct _vshControl { bool imode; /* interactive mode? */ bool quiet; /* quiet mode */ bool timing; /* print timing info? */ + bool no_pkttyagent; /* suppress registration of pkttyagent? */ int debug; /* print debug messages? */ char *logfile; /* log file name */ int log_fd; /* log file descriptor */ -- 2.39.5
From: Jens Schmidt <farblos@vodafonemail.de> Signed-off-by: Jens Schmidt <farblos@vodafonemail.de> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- NEWS.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/NEWS.rst b/NEWS.rst index 0da3bf4d79..d12d473ceb 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,10 @@ v11.4.0 (unreleased) * **New features** + * virsh: Add option ``--no-pkttyagent`` + + That option suppresses registration of pkttyagent with polkitd. + * **Improvements** * **Bug fixes** -- 2.39.5
On Sat, May 17, 2025 at 18:32:17 +0200, Jens Schmidt wrote:
From: Jens Schmidt <farblos@vodafonemail.de>
Signed-off-by: Jens Schmidt <farblos@vodafonemail.de> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- NEWS.rst | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/NEWS.rst b/NEWS.rst index 0da3bf4d79..d12d473ceb 100644 --- a/NEWS.rst +++ b/NEWS.rst @@ -17,6 +17,10 @@ v11.4.0 (unreleased)
* **New features**
+ * virsh: Add option ``--no-pkttyagent`` + + That option suppresses registration of pkttyagent with polkitd.
I'll move this ..
+ * **Improvements**
... under improvements and I'll push this series shortly. Thanks for the patches!
participants (3)
-
Jens Schmidt -
Michal Prívozník -
Peter Krempa