Message ID | 20180817173104.14691-1-marcandre.lureau@redhat.com |
---|---|
State | New |
Headers | show |
Series | vnc: call sasl_server_init() only when required | expand |
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
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 > >
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
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
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;
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(-)