| Message ID | 20120326151340.GA25460@mail.hallyn.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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(-)