Patchwork [1/1] If user doesn't specify a uuid, generate a random one

login
register
mail settings
Submitter Serge E. Hallyn
Date March 26, 2012, 3:13 p.m.
Message ID <20120326151340.GA25460@mail.hallyn.com>
Download mbox | patch
Permalink /patch/148765/
State New
Headers show

Comments

Serge E. Hallyn - March 26, 2012, 3:13 p.m.
Currently, if the user doesn't pass a uuid, the system uuid is set to
all zeros.  This patch generates a random one instead.

Is there a reason to prefer all zeros?  If not, can a patch like this
one be applied?

Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
---
 vl.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
Andreas Färber - March 26, 2012, 3:21 p.m.
Am 26.03.2012 17:13, schrieb Serge E. Hallyn:
> Currently, if the user doesn't pass a uuid, the system uuid is set to
> all zeros.  This patch generates a random one instead.
> 
> Is there a reason to prefer all zeros?

Yes, documented somewhere in the archives, we wanted to have
reproducible defaults in QEMU (cf. MAC address, IP addresses) so that it
doesn't change for each invocation or depending on host.

As a general rule, randomization should be done either explicitly (-uuid
`uuidgen` or -generate-me-a-uuid) or via frontends such as libvirt.

If all zeros causes genuine problems then we should change the default,
taking care of backwards compatibility as usual.

Andreas
Serge E. Hallyn - March 26, 2012, 5:38 p.m.
Quoting Andreas Färber (afaerber@suse.de):
> Am 26.03.2012 17:13, schrieb Serge E. Hallyn:
> > Currently, if the user doesn't pass a uuid, the system uuid is set to
> > all zeros.  This patch generates a random one instead.
> > 
> > Is there a reason to prefer all zeros?
> 
> Yes, documented somewhere in the archives, we wanted to have
> reproducible defaults in QEMU (cf. MAC address, IP addresses) so that it
> doesn't change for each invocation or depending on host.

Thanks.  Though I don't know of a case offhand, I guess I could imagine
a case where a guest's userspace acts differently (and mis-behaves)
based on the random uuid...

> As a general rule, randomization should be done either explicitly (-uuid
> `uuidgen` or -generate-me-a-uuid) or via frontends such as libvirt.
> 
> If all zeros causes genuine problems then we should change the default,
> taking care of backwards compatibility as usual.

The bug this was in reply to is at http://pad.lv/959308 .  IIUC the main
problem is that our crash database uses the uuid, so users need to set
one to report bugs.  (There was also a suggestion that Microsoft requires
it for Logo Certification.)  My suggestion was also to have callers
specify it, but the crash db issue means we'd have to wrap all calls to
qemu.

So thanks - I understand if this patch doesn't make it upstream.  I'll
just carry a patch in our package in that case.

thanks,
-serge
Brian Jackson - March 26, 2012, 6:42 p.m.
On Mon, 26 Mar 2012 10:13:40 -0500, Serge E. Hallyn <serge@hallyn.com>  
wrote:

> Currently, if the user doesn't pass a uuid, the system uuid is set to
> all zeros.  This patch generates a random one instead.
>
> Is there a reason to prefer all zeros?  If not, can a patch like this
> one be applied?
>
> Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
> ---
>  vl.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 112b0e0..2b53b62 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -247,7 +247,9 @@ int nb_numa_nodes;
>  uint64_t node_mem[MAX_NODES];
>  uint64_t node_cpumask[MAX_NODES];
> +#include <uuid/uuid.h>



This adds a hard dep on libuuid (prior to this it's optional and only  
impacts the vdi block driver).



>  uint8_t qemu_uuid[16];
> +bool uuid_set = false;
> static QEMUBootSetHandler *boot_set_handler;
>  static void *boot_set_opaque;
> @@ -3030,6 +3032,7 @@ int main(int argc, char **argv, char **envp)
>                              " Wrong format.\n");
>                      exit(1);
>                  }
> +                uuid_set = true;
>                  break;
>  	    case QEMU_OPTION_option_rom:
>  		if (nb_option_roms >= MAX_OPTION_ROMS) {
> @@ -3200,6 +3203,14 @@ int main(int argc, char **argv, char **envp)
>          exit(0);
>      }
> +    if (!uuid_set) {
> +        uuid_t uuid;
> +        uuid_generate(uuid);
> +        for (i = 0; i < 16; i++) {
> +            qemu_uuid[i] = uuid[i];
> +        }
> +    }
> +
>      /* Open the logfile at this point, if necessary. We can't open the  
> logfile
>       * when encountering either of the logging options (-d or -D)  
> because the
>       * other one may be encountered later on the command line, changing  
> the
Serge E. Hallyn - March 26, 2012, 7:35 p.m.
Quoting Brian Jackson (iggy@theiggy.com):
> On Mon, 26 Mar 2012 10:13:40 -0500, Serge E. Hallyn
> <serge@hallyn.com> wrote:
> 
> >Currently, if the user doesn't pass a uuid, the system uuid is set to
> >all zeros.  This patch generates a random one instead.
> >
> >Is there a reason to prefer all zeros?  If not, can a patch like this
> >one be applied?
> >
> >Signed-off-by: Serge Hallyn <serge.hallyn@canonical.com>
> >---
> > vl.c |   11 +++++++++++
> > 1 files changed, 11 insertions(+), 0 deletions(-)
> >
> >diff --git a/vl.c b/vl.c
> >index 112b0e0..2b53b62 100644
> >--- a/vl.c
> >+++ b/vl.c
> >@@ -247,7 +247,9 @@ int nb_numa_nodes;
> > uint64_t node_mem[MAX_NODES];
> > uint64_t node_cpumask[MAX_NODES];
> >+#include <uuid/uuid.h>
> 
> 
> 
> This adds a hard dep on libuuid (prior to this it's optional and
> only impacts the vdi block driver).

Yup.  If that's a problem we can re-implement our own, if the
change in default behavior is acceptable.

> > uint8_t qemu_uuid[16];
> >+bool uuid_set = false;
> >static QEMUBootSetHandler *boot_set_handler;
> > static void *boot_set_opaque;
> >@@ -3030,6 +3032,7 @@ int main(int argc, char **argv, char **envp)
> >                             " Wrong format.\n");
> >                     exit(1);
> >                 }
> >+                uuid_set = true;
> >                 break;
> > 	    case QEMU_OPTION_option_rom:
> > 		if (nb_option_roms >= MAX_OPTION_ROMS) {
> >@@ -3200,6 +3203,14 @@ int main(int argc, char **argv, char **envp)
> >         exit(0);
> >     }
> >+    if (!uuid_set) {
> >+        uuid_t uuid;
> >+        uuid_generate(uuid);
> >+        for (i = 0; i < 16; i++) {
> >+            qemu_uuid[i] = uuid[i];
> >+        }
> >+    }
> >+
> >     /* Open the logfile at this point, if necessary. We can't
> >open the logfile
> >      * when encountering either of the logging options (-d or -D)
> >because the
> >      * other one may be encountered later on the command line,
> >changing the
Anthony Liguori - March 26, 2012, 7:44 p.m.
On 03/26/2012 10:13 AM, Serge E. Hallyn wrote:
> Currently, if the user doesn't pass a uuid, the system uuid is set to
> all zeros.  This patch generates a random one instead.
>
> Is there a reason to prefer all zeros?  If not, can a patch like this
> one be applied?
>
> Signed-off-by: Serge Hallyn<serge.hallyn@canonical.com>

The other hypervisors don't have a concept of a transient guest like QEMU does.

There is no state preserved between invocations of QEMU.

Setting a random UUID doesn't seem like the right answer to me as it would 
potentially break Windows VMs.

Perhaps if the DMI UUID isn't set, you could look at the root filesystem's UUID?

Not all platforms have a notion of platform UUID so as Ubuntu supports more 
architectures, this problem would have to be dealt with eventually.

Regards,

Anthony Liguori

> ---
>   vl.c |   11 +++++++++++
>   1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 112b0e0..2b53b62 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -247,7 +247,9 @@ int nb_numa_nodes;
>   uint64_t node_mem[MAX_NODES];
>   uint64_t node_cpumask[MAX_NODES];
>
> +#include<uuid/uuid.h>
>   uint8_t qemu_uuid[16];
> +bool uuid_set = false;
>
>   static QEMUBootSetHandler *boot_set_handler;
>   static void *boot_set_opaque;
> @@ -3030,6 +3032,7 @@ int main(int argc, char **argv, char **envp)
>                               " Wrong format.\n");
>                       exit(1);
>                   }
> +                uuid_set = true;
>                   break;
>   	    case QEMU_OPTION_option_rom:
>   		if (nb_option_roms>= MAX_OPTION_ROMS) {
> @@ -3200,6 +3203,14 @@ int main(int argc, char **argv, char **envp)
>           exit(0);
>       }
>
> +    if (!uuid_set) {
> +        uuid_t uuid;
> +        uuid_generate(uuid);
> +        for (i = 0; i<  16; i++) {
> +            qemu_uuid[i] = uuid[i];
> +        }
> +    }
> +
>       /* Open the logfile at this point, if necessary. We can't open the logfile
>        * when encountering either of the logging options (-d or -D) because the
>        * other one may be encountered later on the command line, changing the
Serge E. Hallyn - March 27, 2012, 2:12 a.m.
Quoting Anthony Liguori (anthony@codemonkey.ws):
> On 03/26/2012 10:13 AM, Serge E. Hallyn wrote:
> >Currently, if the user doesn't pass a uuid, the system uuid is set to
> >all zeros.  This patch generates a random one instead.
> >
> >Is there a reason to prefer all zeros?  If not, can a patch like this
> >one be applied?
> >
> >Signed-off-by: Serge Hallyn<serge.hallyn@canonical.com>
> 
> The other hypervisors don't have a concept of a transient guest like QEMU does.

True, and I generally don't like arguments of the form "but (some other)
does it like this," but...

> There is no state preserved between invocations of QEMU.

Right, and I really don't know how many users (besides me) run kvm
by hand, as opposed to using libvirt or testdrive.  Perhaps testdrive
should be extended.

> Setting a random UUID doesn't seem like the right answer to me as it
> would potentially break Windows VMs.
> 
> Perhaps if the DMI UUID isn't set, you could look at the root filesystem's UUID?

Interesting idea.  I don't know enough to know at which point in the code
qemu would know that, but I can take a look.

> Not all platforms have a notion of platform UUID so as Ubuntu
> supports more architectures, this problem would have to be dealt
> with eventually.

So it sounds like in this form certainly it's nacked :)

thanks,
-serge

Patch

diff --git a/vl.c b/vl.c
index 112b0e0..2b53b62 100644
--- a/vl.c
+++ b/vl.c
@@ -247,7 +247,9 @@  int nb_numa_nodes;
 uint64_t node_mem[MAX_NODES];
 uint64_t node_cpumask[MAX_NODES];
 
+#include <uuid/uuid.h>
 uint8_t qemu_uuid[16];
+bool uuid_set = false;
 
 static QEMUBootSetHandler *boot_set_handler;
 static void *boot_set_opaque;
@@ -3030,6 +3032,7 @@  int main(int argc, char **argv, char **envp)
                             " Wrong format.\n");
                     exit(1);
                 }
+                uuid_set = true;
                 break;
 	    case QEMU_OPTION_option_rom:
 		if (nb_option_roms >= MAX_OPTION_ROMS) {
@@ -3200,6 +3203,14 @@  int main(int argc, char **argv, char **envp)
         exit(0);
     }
 
+    if (!uuid_set) {
+        uuid_t uuid;
+        uuid_generate(uuid);
+        for (i = 0; i < 16; i++) {
+            qemu_uuid[i] = uuid[i];
+        }
+    }
+
     /* Open the logfile at this point, if necessary. We can't open the logfile
      * when encountering either of the logging options (-d or -D) because the
      * other one may be encountered later on the command line, changing the