Patchwork [10/14] qapi: add change-vnc-password

login
register
mail settings
Submitter Anthony Liguori
Date Aug. 24, 2011, 6:43 p.m.
Message ID <1314211389-28915-11-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/111403/
State New
Headers show

Comments

Anthony Liguori - Aug. 24, 2011, 6:43 p.m.
This is a new QMP only command that only changes the VNC password.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qapi-schema.json |   13 +++++++++++++
 qmp-commands.hx  |    8 ++++++++
 qmp.c            |   11 ++++++++++-
 3 files changed, 31 insertions(+), 1 deletions(-)
Gerd Hoffmann - Aug. 25, 2011, 9:07 a.m.
On 08/24/11 20:43, Anthony Liguori wrote:
> This is a new QMP only command that only changes the VNC password.

What is wrong with "set_password vnc $secret" ?

cheers,
   Gerd
Anthony Liguori - Aug. 25, 2011, 1:12 p.m.
On 08/25/2011 04:07 AM, Gerd Hoffmann wrote:
> On 08/24/11 20:43, Anthony Liguori wrote:
>> This is a new QMP only command that only changes the VNC password.
>
> What is wrong with "set_password vnc $secret" ?

Overloading a single function to do multiple things makes the function 
hard to use.   Consider the C interface:

void qmp_set_password(QmpSession *sess, const char *protocol,
                       const char *password, bool has_connected,
                       const char *connected)

Since both protocol and connected are strings, you can get hard to debug 
errors by using the string "VNC" instead of "vnc" or "Keep" instead of 
"keep".  There's no obvious way to figure out what strings are valid for 
protocol programatically so if we added rdesktop or something like that, 
the interface would become awkward to use.

Likewise, it's non-intuitive that connected only has one valid value for 
VNC which means that for changing the VNC password, it's just extra 
typing.  Compare that to:

void qmp_change_vnc_password(QmpSession *sess, const char *password)

It's impossible to use wrong.  If we add a qmp_change_rdesktop_password, 
it's easily detectable.

Regards,

Anthony Liguori

>
> cheers,
> Gerd
>
>
Luiz Capitulino - Aug. 25, 2011, 1:33 p.m.
On Wed, 24 Aug 2011 13:43:05 -0500
Anthony Liguori <aliguori@us.ibm.com> wrote:

> This is a new QMP only command that only changes the VNC password.

Isn't this useful in HMP too?

> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  qapi-schema.json |   13 +++++++++++++
>  qmp-commands.hx  |    8 ++++++++
>  qmp.c            |   11 ++++++++++-
>  3 files changed, 31 insertions(+), 1 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f159d81..3b2229f 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -96,3 +96,16 @@
>  { 'command': 'set-blockdev-password',
>    'data': {'device': 'str', 'password': 'str'} }
>  
> +##
> +# @change-vnc-password:
> +#
> +# Change the VNC server password.
> +#
> +# @target:  the new password to use with VNC authentication

The argument name is "password".

> +#
> +# Since: 1.0
> +#
> +# Notes:  An empty password in this command will set the password to the empty
> +#         string.  Existing clients are unaffected by executing this command.
> +##
> +{ 'command': 'change-vnc-password', 'data': {'password': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 909c778..d60f72f 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -868,6 +868,14 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "change-vnc-password",
> +        .args_type  = "password:s",
> +        .params     = "password",
> +        .help       = "set vnc password",
> +        .mhandler.cmd_new = qmp_marshal_input_change_vnc_password,
> +    },
> +
> +    {
>          .name       = "set_password",
>          .args_type  = "protocol:s,password:s,connected:s?",
>          .params     = "protocol password action-if-connected",
> diff --git a/qmp.c b/qmp.c
> index 8aa9c66..f817a88 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -12,9 +12,11 @@
>   */
>  
>  #include "qemu-common.h"
> -#include "sysemu.h"
>  #include "qmp-commands.h"
>  
> +#include "sysemu.h"
> +#include "console.h"
> +
>  NameInfo *qmp_query_name(Error **errp)
>  {
>      NameInfo *info = g_malloc0(sizeof(*info));
> @@ -26,3 +28,10 @@ NameInfo *qmp_query_name(Error **errp)
>  
>      return info;
>  }
> +
> +void qmp_change_vnc_password(const char *password, Error **err)
> +{
> +    if (vnc_display_password(NULL, password) < 0) {
> +        error_set(err, QERR_SET_PASSWD_FAILED);
> +    }
> +}
Anthony Liguori - Sept. 2, 2011, 4:08 p.m.
On 08/25/2011 08:33 AM, Luiz Capitulino wrote:
> On Wed, 24 Aug 2011 13:43:05 -0500
> Anthony Liguori<aliguori@us.ibm.com>  wrote:
>
>> This is a new QMP only command that only changes the VNC password.
>
> Isn't this useful in HMP too?

I think the right design goal for HMP is to minimize the number of 
commands since a human can only remember so much.

>
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>> ---
>>   qapi-schema.json |   13 +++++++++++++
>>   qmp-commands.hx  |    8 ++++++++
>>   qmp.c            |   11 ++++++++++-
>>   3 files changed, 31 insertions(+), 1 deletions(-)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index f159d81..3b2229f 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -96,3 +96,16 @@
>>   { 'command': 'set-blockdev-password',
>>     'data': {'device': 'str', 'password': 'str'} }
>>
>> +##
>> +# @change-vnc-password:
>> +#
>> +# Change the VNC server password.
>> +#
>> +# @target:  the new password to use with VNC authentication
>
> The argument name is "password".

Ack.

Regards,

Anthony Liguori

>
>> +#
>> +# Since: 1.0
>> +#
>> +# Notes:  An empty password in this command will set the password to the empty
>> +#         string.  Existing clients are unaffected by executing this command.
>> +##
>> +{ 'command': 'change-vnc-password', 'data': {'password': 'str'} }
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 909c778..d60f72f 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -868,6 +868,14 @@ Example:
>>   EQMP
>>
>>       {
>> +        .name       = "change-vnc-password",
>> +        .args_type  = "password:s",
>> +        .params     = "password",
>> +        .help       = "set vnc password",
>> +        .mhandler.cmd_new = qmp_marshal_input_change_vnc_password,
>> +    },
>> +
>> +    {
>>           .name       = "set_password",
>>           .args_type  = "protocol:s,password:s,connected:s?",
>>           .params     = "protocol password action-if-connected",
>> diff --git a/qmp.c b/qmp.c
>> index 8aa9c66..f817a88 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -12,9 +12,11 @@
>>    */
>>
>>   #include "qemu-common.h"
>> -#include "sysemu.h"
>>   #include "qmp-commands.h"
>>
>> +#include "sysemu.h"
>> +#include "console.h"
>> +
>>   NameInfo *qmp_query_name(Error **errp)
>>   {
>>       NameInfo *info = g_malloc0(sizeof(*info));
>> @@ -26,3 +28,10 @@ NameInfo *qmp_query_name(Error **errp)
>>
>>       return info;
>>   }
>> +
>> +void qmp_change_vnc_password(const char *password, Error **err)
>> +{
>> +    if (vnc_display_password(NULL, password)<  0) {
>> +        error_set(err, QERR_SET_PASSWD_FAILED);
>> +    }
>> +}
>
>

Patch

diff --git a/qapi-schema.json b/qapi-schema.json
index f159d81..3b2229f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -96,3 +96,16 @@ 
 { 'command': 'set-blockdev-password',
   'data': {'device': 'str', 'password': 'str'} }
 
+##
+# @change-vnc-password:
+#
+# Change the VNC server password.
+#
+# @target:  the new password to use with VNC authentication
+#
+# Since: 1.0
+#
+# Notes:  An empty password in this command will set the password to the empty
+#         string.  Existing clients are unaffected by executing this command.
+##
+{ 'command': 'change-vnc-password', 'data': {'password': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 909c778..d60f72f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -868,6 +868,14 @@  Example:
 EQMP
 
     {
+        .name       = "change-vnc-password",
+        .args_type  = "password:s",
+        .params     = "password",
+        .help       = "set vnc password",
+        .mhandler.cmd_new = qmp_marshal_input_change_vnc_password,
+    },
+
+    {
         .name       = "set_password",
         .args_type  = "protocol:s,password:s,connected:s?",
         .params     = "protocol password action-if-connected",
diff --git a/qmp.c b/qmp.c
index 8aa9c66..f817a88 100644
--- a/qmp.c
+++ b/qmp.c
@@ -12,9 +12,11 @@ 
  */
 
 #include "qemu-common.h"
-#include "sysemu.h"
 #include "qmp-commands.h"
 
+#include "sysemu.h"
+#include "console.h"
+
 NameInfo *qmp_query_name(Error **errp)
 {
     NameInfo *info = g_malloc0(sizeof(*info));
@@ -26,3 +28,10 @@  NameInfo *qmp_query_name(Error **errp)
 
     return info;
 }
+
+void qmp_change_vnc_password(const char *password, Error **err)
+{
+    if (vnc_display_password(NULL, password) < 0) {
+        error_set(err, QERR_SET_PASSWD_FAILED);
+    }
+}