From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57337) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKReY-00027O-Kd for qemu-devel@nongnu.org; Mon, 03 Mar 2014 07:07:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKReS-0004l2-IT for qemu-devel@nongnu.org; Mon, 03 Mar 2014 07:07:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50168) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKReS-0004kh-B5 for qemu-devel@nongnu.org; Mon, 03 Mar 2014 07:07:20 -0500 Message-ID: <1393848446.11589.90.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Mon, 03 Mar 2014 14:07:26 +0200 In-Reply-To: <53145E5D.30704@redhat.com> References: <1393765632-2753-1-git-send-email-marcel.a@redhat.com> <53145E5D.30704@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC V2 0/9] qemu-machine as a QOM object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: peter.maydell@linaro.org, blauwirbel@gmail.com, mdroth@linux.vnet.ibm.com, mst@redhat.com, armbru@redhat.com, mtosatti@redhat.com, agraf@suse.de, ehabkost@redhat.com, qemu-devel@nongnu.org, peter.crosthwaite@petalogix.com, quintela@redhat.com, aliguori@amazon.com, imammedo@redhat.com, scottwood@freescale.com, edgar.iglesias@gmail.com, lcapitulino@redhat.com, afaerber@suse.de, rth@twiddle.net On Mon, 2014-03-03 at 11:50 +0100, Paolo Bonzini wrote: > Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto: > > Most of the "Cc" list is due to patch 8: (Should I send each patch to a different list?) > > machine-opts: replace qemu_opt_get by QOM QemuMachine queries. > > > > Status: > > - machine_opts are mapped into QemuMachineState's properties, > > which can be queried as regular QOM properties. > > - Subclassing QemuMachineClass allows to add a command line > > option specific to a machine type, error mechanism should > > work if this option is used on another machine. (Not tested, on the todo list.) > > - Next big step would be to completely remove the qemu machines initialization > > and replace it by regular QOM type registration. > Having seen all the series, I think we can plan the actual code as follows. Paolo, I really appreciate you are taking the time to go over this! > > Patches 1-3 are fine (except for the missing object_property_add_child > in patch 3). Patch 4 is also fine, but it should refer to the embedded > QEMUMachineInitArgs. I will, thanks. > > The .machine field from QEMUMachineArgs can be removed very early. It > can be replaced with the class of current_machine. Sure. > > Another early refactoring should be to pass ¤t_machine->init_args > to machine->init, not the "args". Problem is that this is a "private field", should I add a getter for it? > > Now here's a possible plan to get rid of QEMUMachineInitArgs: > > 1) Add three wrappers to QemuMachine for reading kernel_filename, > kernel_cmdline, initrd_filename. Unlike object_property_get_str, they > can skip the strdup of the value. This way you don't have to add the > matching free to all uses of the fields. I have nothing against it, but this will break QEMU's unified way to handle object properties, right? Meaning I will have a field of the object state(kernel_filename) and I will add a method like machine_get_field(QemuMachineState) going "around" QOM,,, > > 2) Similarly, add get/set functions (not properties, since these are not > accessible via -machine) for ram_size, boot_order, cpu_model. I am sorry, here you lost me, what do you mean "accessible via -machine"? Maybe that cannot be queried by QOM tree? Those getter/setters are not the same wrappers as above, going around "QOM"? > > 3) Now you can have something like patch 8 in this series. I also need patch 5 that deals with getting a string property with NULL value, > > 4) Thanks to the previous change, there should be no usage of > QEMUMachineInitArgs anymore. Change the machine init function to take a > "QemuMachineState *current_machine". This will touch a lot of files... but needs to be done. > > 5) Remove the current_machine global. There will be few users of > current_machine, and they can look at /machine instead, as mentioned in > the review of patch 3. Agreed > > Does it look feasible? The bulk changes should all be fairly mechanical. I see no reason why not, the main problem I see is the use of those wrappers or setters/getters, I suspect that the usage will be: 1. global QOM query to get the machine 2. Use this wrappers(getters/setters) to do query/alter the machine. Doesn't QOM have another way to do this? Or I am missing something, of course. Thanks again for the review! Marcel > > Paolo >