It does not need a tty to work, it opens its controlling terminal for user
interaction and with this patch even crazy things like this work:

  echo 'list --name' | virsh -q >/dev/null

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/util/virpolkit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index 63bb8133a8aa..7156adc10c0a 100644
--- a/src/util/virpolkit.c
+++ b/src/util/virpolkit.c
@@ -180,9 +180,9 @@ virPolkitAgentCreate(void)
     int outfd = STDOUT_FILENO;
     int errfd = STDERR_FILENO;
 
-    if (!isatty(STDIN_FILENO)) {
+    if (!virPolkitAgentAvailable()) {
         virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
-                       _("Cannot start polkit text agent without a tty"));
+                       _("polkit text authentication agent unavailable"));
         goto error;
     }
 
-- 
2.34.0

Hi Martin!

I recently received a bug report (sorry, not public) about simple operations 
like 'virsh list' hanging when invoked with an internal test tool. I found this 
commit to be the culprit.

On 11/20/21 16:10, Martin Kletzander wrote:
> It does not need a tty to work, it opens its controlling terminal for user
> interaction and with this patch even crazy things like this work:
> 
>    echo 'list --name' | virsh -q >/dev/null

FYI, your crazy thing worked for me without this commit :-).

> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>   src/util/virpolkit.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
> index 63bb8133a8aa..7156adc10c0a 100644
> --- a/src/util/virpolkit.c
> +++ b/src/util/virpolkit.c
> @@ -180,9 +180,9 @@ virPolkitAgentCreate(void)
>       int outfd = STDOUT_FILENO;
>       int errfd = STDERR_FILENO;
>   
> -    if (!isatty(STDIN_FILENO)) {

With the test tool invoking virsh, isatty fails

> +    if (!virPolkitAgentAvailable()) {

but virPolkitAgentAvailable succeeds. pkttyagent is then needlessly spawned with 
no one to talk to.

I haven't been able to cook up a simple reproducer. Not sure if it helps, but 
here's a pstree view of the internal test tool

sshd(15736)---bash(15739)---perl(17717) \
---runtest(17722)---test.sh(17727) \
---virsh(17728)-+-pkttyagent(17730)-+-{gdbus}(17732)
                 |                   |-{gmain}(17731)
                 |                   `-{pkttyagent}(17733)
                 `-{vshEventLoop}(17729)

I'm not familiar with the test tool but have cc'd Julie, who might be able to 
answer any questions about it.

Regards,
Jim

>           virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
> -                       _("Cannot start polkit text agent without a tty"));
> +                       _("polkit text authentication agent unavailable"));
>           goto error;
>       }
>   
> 

On Fri, Dec 10, 2021 at 05:48:03PM -0700, Jim Fehlig wrote:
>Hi Martin!
>
>I recently received a bug report (sorry, not public) about simple operations
>like 'virsh list' hanging when invoked with an internal test tool. I found this
>commit to be the culprit.
>
>On 11/20/21 16:10, Martin Kletzander wrote:
>> It does not need a tty to work, it opens its controlling terminal for user
>> interaction and with this patch even crazy things like this work:
>>
>>    echo 'list --name' | virsh -q >/dev/null
>
>FYI, your crazy thing worked for me without this commit :-).
>

With a polkit access driver, without a polkit agent already running, and
without a polkit rule allowing access without authentication?

>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>   src/util/virpolkit.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
>> index 63bb8133a8aa..7156adc10c0a 100644
>> --- a/src/util/virpolkit.c
>> +++ b/src/util/virpolkit.c
>> @@ -180,9 +180,9 @@ virPolkitAgentCreate(void)
>>       int outfd = STDOUT_FILENO;
>>       int errfd = STDERR_FILENO;
>>
>> -    if (!isatty(STDIN_FILENO)) {
>
>With the test tool invoking virsh, isatty fails
>
>> +    if (!virPolkitAgentAvailable()) {
>
>but virPolkitAgentAvailable succeeds. pkttyagent is then needlessly spawned with
>no one to talk to.
>

Well, pkttyagent does not use stdin at all.  It checks the controlling
terminal and then opens that to be used for both stdout and stdin.
That's why I tried to make it work.

>I haven't been able to cook up a simple reproducer. Not sure if it helps, but
>here's a pstree view of the internal test tool
>
>sshd(15736)---bash(15739)---perl(17717) \
>---runtest(17722)---test.sh(17727) \
>---virsh(17728)-+-pkttyagent(17730)-+-{gdbus}(17732)
>                 |                   |-{gmain}(17731)
>                 |                   `-{pkttyagent}(17733)
>                 `-{vshEventLoop}(17729)
>
>I'm not familiar with the test tool but have cc'd Julie, who might be able to
>answer any questions about it.
>

The thing is that I got our test suite failing because we had the
opposite problem.  Meson is running tests in a new session without a
controlling terminal, however with stdin normally accessible.  Virsh
tries to run it and it fails every time.

I guess there is no harm in also checking if stdin is a tty, I'll send a
patch for that.  Thanks for letting me know!

>Regards,
>Jim
>
>>           virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
>> -                       _("Cannot start polkit text agent without a tty"));
>> +                       _("polkit text authentication agent unavailable"));
>>           goto error;
>>       }
>>
>>
>

On Sat, Dec 11, 2021 at 11:16:13AM +0100, Martin Kletzander wrote:
>On Fri, Dec 10, 2021 at 05:48:03PM -0700, Jim Fehlig wrote:
>>Hi Martin!
>>
>>I recently received a bug report (sorry, not public) about simple operations
>>like 'virsh list' hanging when invoked with an internal test tool. I found this
>>commit to be the culprit.
>>

OK, one more thing though, the fact that pkttyagent is spawned cannot
cause virsh to hang.  If the authentications is not required, then it
will just wait there for a while and then be killed.  If authentication
*is* required, then either you already have an agent running and that
one should be used since we're starting pkttyagent with `--fallback` or
you do not have any agent running in which case virsh list would fail
to connect.  Where does the virsh hang, what's the backtrace?

Anyway, if just adding:

     if (!isatty(STDIN_FILENO))
         return false;

to top of virPolkitAgentAvailable() solves your problem I do not have a
particular issue with it.  In any case I would like to better understand
the issue as just the fact that we're running pkttyagent should not
cause any issues.

Have a nice day,
Martin

On 12/11/21 03:28, Martin Kletzander wrote:
> On Sat, Dec 11, 2021 at 11:16:13AM +0100, Martin Kletzander wrote:
>> On Fri, Dec 10, 2021 at 05:48:03PM -0700, Jim Fehlig wrote:
>>> Hi Martin!
>>>
>>> I recently received a bug report (sorry, not public) about simple operations
>>> like 'virsh list' hanging when invoked with an internal test tool. I found this
>>> commit to be the culprit.
>>>
> 
> OK, one more thing though, the fact that pkttyagent is spawned cannot
> cause virsh to hang.  If the authentications is not required, then it
> will just wait there for a while and then be killed.  If authentication
> *is* required, then either you already have an agent running and that
> one should be used since we're starting pkttyagent with `--fallback` or
> you do not have any agent running in which case virsh list would fail
> to connect.  Where does the virsh hang, what's the backtrace?

The last scenario you describe appears to be the case. virsh fails to connect 
then gets stuck trying to kill off pkttyagent

#0  0x00007f9f07530241 in clock_nanosleep@GLIBC_2.2.5 () from /lib64/libc.so.6
#1  0x00007f9f07535ad3 in nanosleep () from /lib64/libc.so.6
#2  0x00007f9f07f478af in g_usleep () from /usr/lib64/libglib-2.0.so.0
#3  0x00007f9f086694fa in virProcessAbort (pid=367) at ../src/util/virprocess.c:187
#4  0x00007f9f0861ed9b in virCommandAbort (cmd=cmd@entry=0x55a798660c50) at 
../src/util/vircommand.c:2774
#5  0x00007f9f08621478 in virCommandFree (cmd=0x55a798660c50) at 
../src/util/vircommand.c:3061
#6  0x00007f9f08668581 in virPolkitAgentDestroy (agent=0x55a7986426e0) at 
../src/util/virpolkit.c:164
#7  0x000055a797836d93 in virshConnect (ctl=ctl@entry=0x7ffc551dd980, uri=0x0, 
readonly=readonly@entry=false) at ../tools/virsh.c:187
#8  0x000055a797837007 in virshReconnect (ctl=ctl@entry=0x7ffc551dd980, 
name=name@entry=0x0, readonly=<optimized out>, readonly@entry=false, 
force=force@entry=false) at ../tools/virsh.c:223
#9  0x000055a7978371e0 in virshConnectionHandler (ctl=0x7ffc551dd980) at 
../tools/virsh.c:325
#10 0x000055a797880172 in vshCommandRun (ctl=ctl@entry=0x7ffc551dd980, 
cmd=0x55a79865f580) at ../tools/vsh.c:1308
#11 0x000055a7978367b7 in main (argc=2, argv=<optimized out>) at 
../tools/virsh.c:907

Odd thing is, I attached gdb to this virsh process several minutes after 
invoking the test tool that calls 'virsh list'. I can't explain why the process 
is still blocked in g_usleep, which should only have slept for 10 milliseconds. 
Even odder, detaching from the process appears to awaken g_usleep and allows 
process shutdown to continue. The oddness can also be seen in the debug output

2021-12-12 16:35:38.783+0000: 5912: debug : virCommandRunAsync:2629 : About to 
run /usr/bin/pkttyagent --process 5912 --notify-fd 4 --fallback

2021-12-12 16:35:38.787+0000: 5912: debug : virCommandRunAsync:2632 : Command 
result 0, with PID 5914

...
2021-12-12 16:35:38.830+0000: 5912: debug : virProcessAbort:177 : aborting child 
process 5914

2021-12-12 16:35:38.830+0000: 5912: debug : virProcessAbort:185 : trying SIGTERM 
to child process 5914


Attach gdb to the process, observe above backtrace, quit gdb.

2021-12-12 16:44:18.059+0000: 5912: debug : virProcessAbort:195 : trying SIGKILL 
to child process 5914

2021-12-12 16:44:18.061+0000: 5912: debug : virProcessAbort:201 : process has 
ended: fatal signal 9


> Anyway, if just adding:
> 
>      if (!isatty(STDIN_FILENO))
>          return false;

This indeed fixes the regression in the test tool.

> to top of virPolkitAgentAvailable() solves your problem I do not have a
> particular issue with it.  In any case I would like to better understand
> the issue as just the fact that we're running pkttyagent should not
> cause any issues.

Given the above observations, I'm having a difficult time articulating the root 
cause :-).

Regards,
Jim

On Sun, Dec 12, 2021 at 10:40:46AM -0700, Jim Fehlig wrote:
>On 12/11/21 03:28, Martin Kletzander wrote:
>> On Sat, Dec 11, 2021 at 11:16:13AM +0100, Martin Kletzander wrote:
>>> On Fri, Dec 10, 2021 at 05:48:03PM -0700, Jim Fehlig wrote:
>>>> Hi Martin!
>>>>
>>>> I recently received a bug report (sorry, not public) about simple operations
>>>> like 'virsh list' hanging when invoked with an internal test tool. I found this
>>>> commit to be the culprit.
>>>>
>>
>> OK, one more thing though, the fact that pkttyagent is spawned cannot
>> cause virsh to hang.  If the authentications is not required, then it
>> will just wait there for a while and then be killed.  If authentication
>> *is* required, then either you already have an agent running and that
>> one should be used since we're starting pkttyagent with `--fallback` or
>> you do not have any agent running in which case virsh list would fail
>> to connect.  Where does the virsh hang, what's the backtrace?
>
>The last scenario you describe appears to be the case. virsh fails to connect
>then gets stuck trying to kill off pkttyagent
>
>#0  0x00007f9f07530241 in clock_nanosleep@GLIBC_2.2.5 () from /lib64/libc.so.6
>#1  0x00007f9f07535ad3 in nanosleep () from /lib64/libc.so.6
>#2  0x00007f9f07f478af in g_usleep () from /usr/lib64/libglib-2.0.so.0
>#3  0x00007f9f086694fa in virProcessAbort (pid=367) at ../src/util/virprocess.c:187
>#4  0x00007f9f0861ed9b in virCommandAbort (cmd=cmd@entry=0x55a798660c50) at
>../src/util/vircommand.c:2774
>#5  0x00007f9f08621478 in virCommandFree (cmd=0x55a798660c50) at
>../src/util/vircommand.c:3061
>#6  0x00007f9f08668581 in virPolkitAgentDestroy (agent=0x55a7986426e0) at
>../src/util/virpolkit.c:164
>#7  0x000055a797836d93 in virshConnect (ctl=ctl@entry=0x7ffc551dd980, uri=0x0,
>readonly=readonly@entry=false) at ../tools/virsh.c:187
>#8  0x000055a797837007 in virshReconnect (ctl=ctl@entry=0x7ffc551dd980,
>name=name@entry=0x0, readonly=<optimized out>, readonly@entry=false,
>force=force@entry=false) at ../tools/virsh.c:223
>#9  0x000055a7978371e0 in virshConnectionHandler (ctl=0x7ffc551dd980) at
>../tools/virsh.c:325
>#10 0x000055a797880172 in vshCommandRun (ctl=ctl@entry=0x7ffc551dd980,
>cmd=0x55a79865f580) at ../tools/vsh.c:1308
>#11 0x000055a7978367b7 in main (argc=2, argv=<optimized out>) at
>../tools/virsh.c:907
>
>Odd thing is, I attached gdb to this virsh process several minutes after
>invoking the test tool that calls 'virsh list'. I can't explain why the process
>is still blocked in g_usleep, which should only have slept for 10 milliseconds.
>Even odder, detaching from the process appears to awaken g_usleep and allows
>process shutdown to continue. The oddness can also be seen in the debug output
>
>2021-12-12 16:35:38.783+0000: 5912: debug : virCommandRunAsync:2629 : About to
>run /usr/bin/pkttyagent --process 5912 --notify-fd 4 --fallback
>
>2021-12-12 16:35:38.787+0000: 5912: debug : virCommandRunAsync:2632 : Command
>result 0, with PID 5914
>
>...
>2021-12-12 16:35:38.830+0000: 5912: debug : virProcessAbort:177 : aborting child
>process 5914
>
>2021-12-12 16:35:38.830+0000: 5912: debug : virProcessAbort:185 : trying SIGTERM
>to child process 5914
>
>
>Attach gdb to the process, observe above backtrace, quit gdb.
>
>2021-12-12 16:44:18.059+0000: 5912: debug : virProcessAbort:195 : trying SIGKILL
>to child process 5914
>
>2021-12-12 16:44:18.061+0000: 5912: debug : virProcessAbort:201 : process has
>ended: fatal signal 9
>

The stuck g_usleep() is weird.  Isn't there a tremendous load on the
machine?  I can't think of much else.

Also I am looking at the pkttyagent code and it looks like it blocks the
first SIGTERM and sending two of them should help in that case, but if
we want to wait for few ms between them, than we;ll be in the same
pickle.

>
>> Anyway, if just adding:
>>
>>      if (!isatty(STDIN_FILENO))
>>          return false;
>
>This indeed fixes the regression in the test tool.
>

That just means that it won't start the agent.  Let's do this, but I
would really, *really* like to figure out what the heck is happening
there, because there has to be something wrong and it might just be
waiting around the corner for us and bite us in the back in a year or
so.  Although I understand how improbable that is.

>> to top of virPolkitAgentAvailable() solves your problem I do not have a
>> particular issue with it.  In any case I would like to better understand
>> the issue as just the fact that we're running pkttyagent should not
>> cause any issues.
>
>Given the above observations, I'm having a difficult time articulating the root
>cause :-).
>
>Regards,
>Jim
>

On 12/12/21 14:15, Martin Kletzander wrote:
> On Sun, Dec 12, 2021 at 10:40:46AM -0700, Jim Fehlig wrote:
>> On 12/11/21 03:28, Martin Kletzander wrote:
>>> On Sat, Dec 11, 2021 at 11:16:13AM +0100, Martin Kletzander wrote:
>>>> On Fri, Dec 10, 2021 at 05:48:03PM -0700, Jim Fehlig wrote:
>>>>> Hi Martin!
>>>>>
>>>>> I recently received a bug report (sorry, not public) about simple operations
>>>>> like 'virsh list' hanging when invoked with an internal test tool. I found 
>>>>> this
>>>>> commit to be the culprit.
>>>>>
>>>
>>> OK, one more thing though, the fact that pkttyagent is spawned cannot
>>> cause virsh to hang.  If the authentications is not required, then it
>>> will just wait there for a while and then be killed.  If authentication
>>> *is* required, then either you already have an agent running and that
>>> one should be used since we're starting pkttyagent with `--fallback` or
>>> you do not have any agent running in which case virsh list would fail
>>> to connect.  Where does the virsh hang, what's the backtrace?
>>
>> The last scenario you describe appears to be the case. virsh fails to connect
>> then gets stuck trying to kill off pkttyagent
>>
>> #0  0x00007f9f07530241 in clock_nanosleep@GLIBC_2.2.5 () from /lib64/libc.so.6
>> #1  0x00007f9f07535ad3 in nanosleep () from /lib64/libc.so.6
>> #2  0x00007f9f07f478af in g_usleep () from /usr/lib64/libglib-2.0.so.0
>> #3  0x00007f9f086694fa in virProcessAbort (pid=367) at 
>> ../src/util/virprocess.c:187
>> #4  0x00007f9f0861ed9b in virCommandAbort (cmd=cmd@entry=0x55a798660c50) at
>> ../src/util/vircommand.c:2774
>> #5  0x00007f9f08621478 in virCommandFree (cmd=0x55a798660c50) at
>> ../src/util/vircommand.c:3061
>> #6  0x00007f9f08668581 in virPolkitAgentDestroy (agent=0x55a7986426e0) at
>> ../src/util/virpolkit.c:164
>> #7  0x000055a797836d93 in virshConnect (ctl=ctl@entry=0x7ffc551dd980, uri=0x0,
>> readonly=readonly@entry=false) at ../tools/virsh.c:187
>> #8  0x000055a797837007 in virshReconnect (ctl=ctl@entry=0x7ffc551dd980,
>> name=name@entry=0x0, readonly=<optimized out>, readonly@entry=false,
>> force=force@entry=false) at ../tools/virsh.c:223
>> #9  0x000055a7978371e0 in virshConnectionHandler (ctl=0x7ffc551dd980) at
>> ../tools/virsh.c:325
>> #10 0x000055a797880172 in vshCommandRun (ctl=ctl@entry=0x7ffc551dd980,
>> cmd=0x55a79865f580) at ../tools/vsh.c:1308
>> #11 0x000055a7978367b7 in main (argc=2, argv=<optimized out>) at
>> ../tools/virsh.c:907
>>
>> Odd thing is, I attached gdb to this virsh process several minutes after
>> invoking the test tool that calls 'virsh list'. I can't explain why the process
>> is still blocked in g_usleep, which should only have slept for 10 milliseconds.
>> Even odder, detaching from the process appears to awaken g_usleep and allows
>> process shutdown to continue. The oddness can also be seen in the debug output
>>
>> 2021-12-12 16:35:38.783+0000: 5912: debug : virCommandRunAsync:2629 : About to
>> run /usr/bin/pkttyagent --process 5912 --notify-fd 4 --fallback
>>
>> 2021-12-12 16:35:38.787+0000: 5912: debug : virCommandRunAsync:2632 : Command
>> result 0, with PID 5914
>>
>> ...
>> 2021-12-12 16:35:38.830+0000: 5912: debug : virProcessAbort:177 : aborting child
>> process 5914
>>
>> 2021-12-12 16:35:38.830+0000: 5912: debug : virProcessAbort:185 : trying SIGTERM
>> to child process 5914
>>
>>
>> Attach gdb to the process, observe above backtrace, quit gdb.
>>
>> 2021-12-12 16:44:18.059+0000: 5912: debug : virProcessAbort:195 : trying SIGKILL
>> to child process 5914
>>
>> 2021-12-12 16:44:18.061+0000: 5912: debug : virProcessAbort:201 : process has
>> ended: fatal signal 9
>>
> 
> The stuck g_usleep() is weird.  Isn't there a tremendous load on the
> machine?  I can't think of much else.

The machine is mostly idle.

> Also I am looking at the pkttyagent code and it looks like it blocks the
> first SIGTERM and sending two of them should help in that case, but if
> we want to wait for few ms between them, than we;ll be in the same
> pickle.
> 
>>
>>> Anyway, if just adding:
>>>
>>>      if (!isatty(STDIN_FILENO))
>>>          return false;
>>
>> This indeed fixes the regression in the test tool.
>>
> 
> That just means that it won't start the agent.  Let's do this, but I
> would really, *really* like to figure out what the heck is happening
> there, because there has to be something wrong and it might just be
> waiting around the corner for us and bite us in the back in a year or
> so.  Although I understand how improbable that is.

Do you have additional suggestions that may help us gain a better understanding 
of the problem?

Regards,
Jim

My idea was that running pkttyagent unconditionally, modulo checks that
pkttyagent itself does to make sure it does not fail, is not going to be an
issue turned out to be wrong.  Adding back the original check for stdin being a
tty helps in some testing scenarios as reported by Jim Fehlig and does not
really cause any issues.  I originally wanted it in because it also made
pkttyagent auth work with redirected input into virsh (with a connection that
requires polkit authentication and without a session-wide polkit tty agent,
basically making pkttyagent necessary to succeed).  But anyone running virsh
like that is asking for problems already anyway =)

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 src/util/virpolkit.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index 8970de192fe1..bb68b5a59614 100644
--- a/src/util/virpolkit.c
+++ b/src/util/virpolkit.c
@@ -237,6 +237,9 @@ virPolkitAgentAvailable(void)
     const char *termid = ctermid(NULL);
     VIR_AUTOCLOSE fd = -1;
 
+    if (!isatty(STDIN_FILENO))
+        return false;
+
     if (!virFileIsExecutable(PKTTYAGENT))
         return false;
 
-- 
2.34.1

On Sat, Dec 11, 2021 at 02:23:11PM +0100, Martin Kletzander wrote:
>My idea was that running pkttyagent unconditionally, modulo checks that
>pkttyagent itself does to make sure it does not fail, is not going to be an
>issue turned out to be wrong.  Adding back the original check for stdin being a
>tty helps in some testing scenarios as reported by Jim Fehlig and does not
>really cause any issues.  I originally wanted it in because it also made
>pkttyagent auth work with redirected input into virsh (with a connection that
>requires polkit authentication and without a session-wide polkit tty agent,
>basically making pkttyagent necessary to succeed).  But anyone running virsh
>like that is asking for problems already anyway =)
>
>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>---

Of course I added a note that this would be a patch that can go in if we
figure out the real root cause, then sent it to a wrong address, then
formatted it again properly, sent it to the right address ad forgot to
add the note again 🤦

> src/util/virpolkit.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
>index 8970de192fe1..bb68b5a59614 100644
>--- a/src/util/virpolkit.c
>+++ b/src/util/virpolkit.c
>@@ -237,6 +237,9 @@ virPolkitAgentAvailable(void)
>     const char *termid = ctermid(NULL);
>     VIR_AUTOCLOSE fd = -1;
>
>+    if (!isatty(STDIN_FILENO))
>+        return false;
>+
>     if (!virFileIsExecutable(PKTTYAGENT))
>         return false;
>
>-- 
>2.34.1
>

On 12/11/21 14:23, Martin Kletzander wrote:
> My idea was that running pkttyagent unconditionally, modulo checks that
> pkttyagent itself does to make sure it does not fail, is not going to be an
> issue turned out to be wrong.  Adding back the original check for stdin being a
> tty helps in some testing scenarios as reported by Jim Fehlig and does not
> really cause any issues.  I originally wanted it in because it also made
> pkttyagent auth work with redirected input into virsh (with a connection that
> requires polkit authentication and without a session-wide polkit tty agent,
> basically making pkttyagent necessary to succeed).  But anyone running virsh
> like that is asking for problems already anyway =)
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
>  src/util/virpolkit.c | 3 +++
>  1 file changed, 3 insertions(+)

In case you want to push this:

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

But I'd really like to get to the bottom of this. Why does sleep()
suspends for that long and why isn't polkit-agent killed?

Michal

On 12/17/21 7:01 AM, Michal Prívozník wrote:
> On 12/11/21 14:23, Martin Kletzander wrote:
>> My idea was that running pkttyagent unconditionally, modulo checks that
>> pkttyagent itself does to make sure it does not fail, is not going to be an
>> issue turned out to be wrong.  Adding back the original check for stdin being a
>> tty helps in some testing scenarios as reported by Jim Fehlig and does not
>> really cause any issues.  I originally wanted it in because it also made
>> pkttyagent auth work with redirected input into virsh (with a connection that
>> requires polkit authentication and without a session-wide polkit tty agent,
>> basically making pkttyagent necessary to succeed).  But anyone running virsh
>> like that is asking for problems already anyway =)
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>>   src/util/virpolkit.c | 3 +++
>>   1 file changed, 3 insertions(+)
> 
> In case you want to push this:
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

And

Tested-by: Jim Fehlig <jfehlig@suse.com>

> 
> But I'd really like to get to the bottom of this. Why does sleep()
> suspends for that long and why isn't polkit-agent killed?

I got sidetracked before I could dig deeper, however I did notice the test 
process and all children were in a stopped state per /proc/<pid>/status. I sent 
SIGCONT to the processes and the test successfully completed. I'm baffled why 
these processes become stopped when pkttyagent is in the picture and work fine 
otherwise.

Jim

On 12/17/21 9:25 AM, Jim Fehlig wrote:
> On 12/17/21 7:01 AM, Michal Prívozník wrote:
>> On 12/11/21 14:23, Martin Kletzander wrote:
>>> My idea was that running pkttyagent unconditionally, modulo checks that
>>> pkttyagent itself does to make sure it does not fail, is not going to be an
>>> issue turned out to be wrong.  Adding back the original check for stdin being a
>>> tty helps in some testing scenarios as reported by Jim Fehlig and does not
>>> really cause any issues.  I originally wanted it in because it also made
>>> pkttyagent auth work with redirected input into virsh (with a connection that
>>> requires polkit authentication and without a session-wide polkit tty agent,
>>> basically making pkttyagent necessary to succeed).  But anyone running virsh
>>> like that is asking for problems already anyway =)
>>>
>>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>>> ---
>>>   src/util/virpolkit.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>
>> In case you want to push this:
>>
>> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> And
> 
> Tested-by: Jim Fehlig <jfehlig@suse.com>
> 
>>
>> But I'd really like to get to the bottom of this. Why does sleep()
>> suspends for that long and why isn't polkit-agent killed?
> 
> I got sidetracked before I could dig deeper, however I did notice the test 
> process and all children were in a stopped state per /proc/<pid>/status. I sent 
> SIGCONT to the processes and the test successfully completed. I'm baffled why 
> these processes become stopped when pkttyagent is in the picture and work fine 
> otherwise.

I forgot to mention, it's likely a problem in the test framework code, which is 
apparently unmaintained by still used by SUSE QA. "I got sidetracked" is a nice 
way to say I didn't have a lot of motivation to read through a gob of old, 
unfamiliar perl code :-).

Jim

On a Sunday in 2021, Martin Kletzander wrote:
>It does not need a tty to work, it opens its controlling terminal for user
>interaction and with this patch even crazy things like this work:
>
>  echo 'list --name' | virsh -q >/dev/null
>
>Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>---
> src/util/virpolkit.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano