On Mon, Jan 31, 2011 at 02:43:19PM -0600, Anthony Liguori wrote:
commit 52c18be9e99dabe295321153fda7fce9f76647ac introduced a regression in the
change vnc password command that changed the behavior of setting the VNC
password to an empty string from disabling login to disabling authentication.
This commit refactors the code to eliminate this overloaded semantics in
vnc_display_password and instead introduces the vnc_display_disable_login. The
monitor implementation then determines the behavior of an empty or missing
string.
Personally I think this is a little overkill& just reverting the
original patch was fine, but from a functional POV your patch
produces the same results, so I won't argue.
Recently, a set_password command was added that allows both the Spice and VNC
password to be set. This command has not shown up in a release yet so the
behavior is not yet defined.
This patch proposes that an empty password be treated as an empty password with
no special handling. For specifically disabling login, I believe a new command
should be introduced instead of overloading semantics.
Agreed, if some mgmt app does need to change this kind of thin
on the fly, they'll likely want more than just a toggle between
AUTH_NONE/AUTH_VNC too. eg There's AUTH_SASL, which is the only
VNC auth scheme with any real security, and the psuedo auth
schemes for providing the TLS encryption/certificate support.
I'm not sure how Spice handles this but I would recommend that we have Spice
and VNC have consistent semantics here for the 0.14.0 release.
Sounds like a very good idea.
Reported-by: Neil Wilson<address@hidden>
Signed-off-by: Anthony Liguori<address@hidden>
diff --git a/console.h b/console.h
index 3157330..f4e4741 100644
--- a/console.h
+++ b/console.h
@@ -369,6 +369,7 @@ void vnc_display_init(DisplayState *ds);
void vnc_display_close(DisplayState *ds);
int vnc_display_open(DisplayState *ds, const char *display);
int vnc_display_password(DisplayState *ds, const char *password);
+int vnc_display_disable_login(DisplayState *ds);
int vnc_display_pw_expire(DisplayState *ds, time_t expires);
void do_info_vnc_print(Monitor *mon, const QObject *data);
void do_info_vnc(Monitor *mon, QObject **ret_data);
diff --git a/monitor.c b/monitor.c
index c5f54f4..24ed971 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1018,6 +1018,13 @@ static int do_quit(Monitor *mon, const QDict *qdict,
QObject **ret_data)
static int change_vnc_password(const char *password)
{
+ if (!password || !password[0]) {
+ if (vnc_display_disable_login(NULL)) {
+ qerror_report(QERR_SET_PASSWD_FAILED);
+ return -1;
+ }
+ }
+
if (vnc_display_password(NULL, password)< 0) {
qerror_report(QERR_SET_PASSWD_FAILED);
return -1;
@@ -1117,6 +1124,8 @@ static int set_password(Monitor *mon, const QDict *qdict,
QObject **ret_data)
qerror_report(QERR_INVALID_PARAMETER, "connected");
return -1;
}
+ /* Note that setting an empty password will not disable login through
+ * this interface. */
rc = vnc_display_password(NULL, password);
if (rc != 0) {
qerror_report(QERR_SET_PASSWD_FAILED);
diff --git a/ui/vnc.c b/ui/vnc.c
index 495d6d6..73e7ffa 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2484,6 +2484,24 @@ void vnc_display_close(DisplayState *ds)
#endif
}
+int vnc_display_disable_login(DisplayState *ds)
+{
+ VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
+
+ if (!vs) {
+ return -1;
+ }
+
+ if (vs->password) {
+ qemu_free(vs->password);
+ }
+
+ vs->password = NULL;
+ vs->auth = VNC_AUTH_VNC;
+
+ return 0;
+}
+
int vnc_display_password(DisplayState *ds, const char *password)
{
VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
@@ -2492,19 +2510,18 @@ int vnc_display_password(DisplayState *ds, const char
*password)
return -1;
}
+ if (!password) {
+ /* This is not the intention of this interface but err on the side
+ of being safe */
+ return vnc_display_disable_login(ds);
+ }
+
if (vs->password) {
qemu_free(vs->password);
vs->password = NULL;
}
- if (password&& password[0]) {
- if (!(vs->password = qemu_strdup(password)))
- return -1;
- if (vs->auth == VNC_AUTH_NONE) {
- vs->auth = VNC_AUTH_VNC;
- }
- } else {
- vs->auth = VNC_AUTH_NONE;
- }
+ vs->password = qemu_strdup(password);
+ vs->auth = VNC_AUTH_VNC;
return 0;
}
Looks good, assuming the addition of the missing 'return 0' you already
mentioned