4:09 p.m.
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
4:09 p.m.
New subject: [PATCH 1/1] virsh: Add option '--no-pkttyagent'
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
3:24 a.m.
New subject: [PATCH 1/1] virsh: Add option '--no-pkttyagent'
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.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/757
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(a)redhat.com>
when sending the updated version.
4:50 p.m.
New subject: [PATCH 1/1] virsh: Add option '--no-pkttyagent'
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?
7:54 a.m.
New subject: [PATCH 1/1] virsh: Add option '--no-pkttyagent'
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
11:32 a.m.
New subject: [PATCHv2 0/2] virsh: Add option '--no-pkttyagent'
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(a)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/Y5...
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
11:33 a.m.
New subject: [PATCHv2 1/2] virsh: Add option '--no-pkttyagent'
From: Jens Schmidt <farblos(a)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(a)vodafonemail.de>
Reviewed-by: Peter Krempa <pkrempa(a)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
11:33 a.m.
New subject: [PATCHv2 2/2] NEWS: Mention new option '--no-pkttyagent'
From: Jens Schmidt <farblos(a)vodafonemail.de>
Signed-off-by: Jens Schmidt <farblos(a)vodafonemail.de>
Reviewed-by: Peter Krempa <pkrempa(a)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
2:10 a.m.
New subject: [PATCHv2 2/2] NEWS: Mention new option '--no-pkttyagent'
On Sat, May 17, 2025 at 18:32:17 +0200, Jens Schmidt wrote:
From: Jens Schmidt <farblos(a)vodafonemail.de>
Signed-off-by: Jens Schmidt <farblos(a)vodafonemail.de>
Reviewed-by: Peter Krempa <pkrempa(a)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!
14
days inactive
28
days old
8 comments
3 participants
participants (3)
-
Jens Schmidt -
Michal Prívozník -
Peter Krempa