[Qemu-devel] [PATCH] vnc: call sasl_server_init() only when required

[Qemu-devel] [PATCH] vnc: call sasl_server_init() only when required

[Marc-André Lureau]

VNC server is calling sasl_server_init() during startup of QEMU, even
if SASL auth has not been enabled.

This may create undesirable warnings like "Could not find keytab file:
/etc/qemu/krb5.tab" when the user didn't configure SASL on host and
started VNC server.

Instead, only initialize SASL when needed. Note that HMP/QMP "change
vnc" calls vnc_display_open() again, which will initialize SASL if
needed.

Related to:
https://bugzilla.redhat.com/show_bug.cgi?id=1609327

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/vnc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 359693238b..fc507d7f36 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -4054,7 +4054,7 @@ void vnc_display_open(const char *id, Error **errp)
     trace_vnc_auth_init(vd, 1, vd->ws_auth, vd->ws_subauth);
 
 #ifdef CONFIG_VNC_SASL
-    if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
+    if (sasl && ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK)) {
         error_setg(errp, "Failed to initialize SASL auth: %s",
                    sasl_errstring(saslErr, NULL, NULL));
         goto fail;
-- 
2.18.0.547.g1d89318c48

Re: [Qemu-devel] [PATCH] vnc: call sasl_server_init() only when required

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180817173104.14691-1-marcandre.lureau@redhat.com
Subject: [Qemu-devel] [PATCH] vnc: call sasl_server_init() only when required

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c0c5fbce8a vnc: call sasl_server_init() only when required

=== OUTPUT BEGIN ===
Checking PATCH 1/1: vnc: call sasl_server_init() only when required...
ERROR: do not use assignment in if condition
#35: FILE: ui/vnc.c:4057:
+    if (sasl && ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK)) {

total: 1 errors, 0 warnings, 8 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] vnc: call sasl_server_init() only when required

[Marc-André Lureau]

ping
On Fri, Aug 17, 2018 at 7:32 PM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> VNC server is calling sasl_server_init() during startup of QEMU, even
> if SASL auth has not been enabled.
>
> This may create undesirable warnings like "Could not find keytab file:
> /etc/qemu/krb5.tab" when the user didn't configure SASL on host and
> started VNC server.
>
> Instead, only initialize SASL when needed. Note that HMP/QMP "change
> vnc" calls vnc_display_open() again, which will initialize SASL if
> needed.
>
> Related to:
> https://bugzilla.redhat.com/show_bug.cgi?id=1609327
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/vnc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 359693238b..fc507d7f36 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -4054,7 +4054,7 @@ void vnc_display_open(const char *id, Error **errp)
>      trace_vnc_auth_init(vd, 1, vd->ws_auth, vd->ws_subauth);
>
>  #ifdef CONFIG_VNC_SASL
> -    if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
> +    if (sasl && ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK)) {
>          error_setg(errp, "Failed to initialize SASL auth: %s",
>                     sasl_errstring(saslErr, NULL, NULL));
>          goto fail;
> --
> 2.18.0.547.g1d89318c48
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] vnc: call sasl_server_init() only when required

[Gerd Hoffmann]

On Tue, Aug 28, 2018 at 12:57:56PM +0200, Marc-André Lureau wrote:
> ping

patchew flagged a codestyle issue, I've expected to see a v2 with that
fixed ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] vnc: call sasl_server_init() only when required

[Daniel P. Berrangé]

On Fri, Aug 17, 2018 at 07:31:03PM +0200, Marc-André Lureau wrote:
> VNC server is calling sasl_server_init() during startup of QEMU, even
> if SASL auth has not been enabled.
> 
> This may create undesirable warnings like "Could not find keytab file:
> /etc/qemu/krb5.tab" when the user didn't configure SASL on host and
> started VNC server.
> 
> Instead, only initialize SASL when needed. Note that HMP/QMP "change
> vnc" calls vnc_display_open() again, which will initialize SASL if
> needed.

I could have sworn we had a way to change the VNC auth method on the
fly without restarting the whole server, but I can't find it now, so
must have been imagining it :-)

> 
> Related to:
> https://bugzilla.redhat.com/show_bug.cgi?id=1609327
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/vnc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 359693238b..fc507d7f36 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -4054,7 +4054,7 @@ void vnc_display_open(const char *id, Error **errp)
>      trace_vnc_auth_init(vd, 1, vd->ws_auth, vd->ws_subauth);
>  
>  #ifdef CONFIG_VNC_SASL
> -    if ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK) {
> +    if (sasl && ((saslErr = sasl_server_init(NULL, "qemu")) != SASL_OK)) {
>          error_setg(errp, "Failed to initialize SASL auth: %s",
>                     sasl_errstring(saslErr, NULL, NULL));
>          goto fail;

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|