Re: [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early

2013-08-20 Thread Markus Armbruster
Andreas Färber afaer...@suse.de writes:

 Am 19.08.2013 11:35, schrieb Markus Armbruster:
 Andreas Färber afaer...@suse.de writes:
 
 Am 16.08.2013 15:18, schrieb arm...@redhat.com:
 From: Markus Armbruster arm...@redhat.com

 I'd like to access QEMUMachine from a QEMUMachine init() method, which
 is currently not possible.  Instead of passing it as an argument, I
 simply set current_machine earlier.

 We had such a patch for CPU hot-add and decided against doing this.
 Currently current_machine != signals that it has been initialized. And
 
 Does any code actually depend on this undocumented condition?  I found
 none.

 I didn't audit. Currently the users are limited to vl.c itself,
 device-hotplug.c for block_default_type and qmp.c for hot_add_cpu. pc.c
 feels odd in that mix.

 [...]
 Can't you pass either QEMUMachine or the specific fields needed from PC
 code to those SMBIOS functions? You did add a bool argument.
 
 Can't see how to do that without passing the machine to QEMUMachine
 method init(), which involves touching all boards.  I doubt that's a
 good idea, but if you insist, I can do it.

 Isn't that exactly what QEMUMachineArgs was meant to address? :)
 Had a look at your don't-explode patches and they looked good.

I could give it a try, but to be honest, I'm reluctant to base new work
on a series that has been ignored by all committers for more than two
months.



Re: [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early

2013-08-19 Thread Markus Armbruster
Andreas Färber afaer...@suse.de writes:

 Am 16.08.2013 15:18, schrieb arm...@redhat.com:
 From: Markus Armbruster arm...@redhat.com
 
 I'd like to access QEMUMachine from a QEMUMachine init() method, which
 is currently not possible.  Instead of passing it as an argument, I
 simply set current_machine earlier.

 We had such a patch for CPU hot-add and decided against doing this.
 Currently current_machine != signals that it has been initialized. And

Does any code actually depend on this undocumented condition?  I found
none.

 generally we have been trying to get away from accessing globals from
 random parts of code.

Global state need to be managed with care.  Global variables have their
place in that.

In my experience, the most common kind of carelessness involving global
variables is indisciplined *updating* of global state via global
variables.

Unchanging global state is relatively harmless, and referring to it via
global variables is often the easiest and most obvious way to manage
such state.

 Can't you pass either QEMUMachine or the specific fields needed from PC
 code to those SMBIOS functions? You did add a bool argument.

Can't see how to do that without passing the machine to QEMUMachine
method init(), which involves touching all boards.  I doubt that's a
good idea, but if you insist, I can do it.



Re: [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early

2013-08-19 Thread Andreas Färber
Am 19.08.2013 11:35, schrieb Markus Armbruster:
 Andreas Färber afaer...@suse.de writes:
 
 Am 16.08.2013 15:18, schrieb arm...@redhat.com:
 From: Markus Armbruster arm...@redhat.com

 I'd like to access QEMUMachine from a QEMUMachine init() method, which
 is currently not possible.  Instead of passing it as an argument, I
 simply set current_machine earlier.

 We had such a patch for CPU hot-add and decided against doing this.
 Currently current_machine != signals that it has been initialized. And
 
 Does any code actually depend on this undocumented condition?  I found
 none.

I didn't audit. Currently the users are limited to vl.c itself,
device-hotplug.c for block_default_type and qmp.c for hot_add_cpu. pc.c
feels odd in that mix.

[...]
 Can't you pass either QEMUMachine or the specific fields needed from PC
 code to those SMBIOS functions? You did add a bool argument.
 
 Can't see how to do that without passing the machine to QEMUMachine
 method init(), which involves touching all boards.  I doubt that's a
 good idea, but if you insist, I can do it.

Isn't that exactly what QEMUMachineArgs was meant to address? :)
Had a look at your don't-explode patches and they looked good.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg



Re: [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early

2013-08-17 Thread Andreas Färber
Am 16.08.2013 15:18, schrieb arm...@redhat.com:
 From: Markus Armbruster arm...@redhat.com
 
 I'd like to access QEMUMachine from a QEMUMachine init() method, which
 is currently not possible.  Instead of passing it as an argument, I
 simply set current_machine earlier.

We had such a patch for CPU hot-add and decided against doing this.
Currently current_machine != signals that it has been initialized. And
generally we have been trying to get away from accessing globals from
random parts of code.

Can't you pass either QEMUMachine or the specific fields needed from PC
code to those SMBIOS functions? You did add a bool argument.

Andreas

 
 Signed-off-by: Markus Armbruster arm...@redhat.com
 Reviewed-by: Eric Blake ebl...@redhat.com
 ---
  vl.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)
 
 diff --git a/vl.c b/vl.c
 index ba3084c..258e164 100644
 --- a/vl.c
 +++ b/vl.c
 @@ -3897,6 +3897,7 @@ int main(int argc, char **argv, char **envp)
  fprintf(stderr, No machine found.\n);
  exit(1);
  }
 +current_machine = machine;
  
  if (machine-hw_version) {
  qemu_set_version(machine-hw_version);
 @@ -4325,8 +4326,6 @@ int main(int argc, char **argv, char **envp)
  
  set_numa_modes();
  
 -current_machine = machine;
 -
  /* init USB devices */
  if (usb_enabled(false)) {
  if (foreach_device_config(DEV_USB, usb_parse)  0)
 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg



[Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early

2013-08-16 Thread armbru
From: Markus Armbruster arm...@redhat.com

I'd like to access QEMUMachine from a QEMUMachine init() method, which
is currently not possible.  Instead of passing it as an argument, I
simply set current_machine earlier.

Signed-off-by: Markus Armbruster arm...@redhat.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
 vl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index ba3084c..258e164 100644
--- a/vl.c
+++ b/vl.c
@@ -3897,6 +3897,7 @@ int main(int argc, char **argv, char **envp)
 fprintf(stderr, No machine found.\n);
 exit(1);
 }
+current_machine = machine;
 
 if (machine-hw_version) {
 qemu_set_version(machine-hw_version);
@@ -4325,8 +4326,6 @@ int main(int argc, char **argv, char **envp)
 
 set_numa_modes();
 
-current_machine = machine;
-
 /* init USB devices */
 if (usb_enabled(false)) {
 if (foreach_device_config(DEV_USB, usb_parse)  0)
-- 
1.8.1.4