On Fri, 7 Jul 2017 10:13:00 -0300 "Eduardo Habkost" <ehabk...@redhat.com> wrote:
> On Fri, Jul 07, 2017 at 01:33:20PM +0200, Igor Mammedov wrote: > > On Tue, 4 Jul 2017 19:08:44 +0100 > > Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> wrote: > > > > > On 03/07/17 10:39, Igor Mammedov wrote: > > > > > > > On Thu, 29 Jun 2017 15:07:19 +0100 > > > > Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> wrote: > > > > > > > >> When looking to instantiate a TYPE_FW_CFG_MEM or TYPE_FW_CFG_IO device > > > >> to be > > > >> able to wire it up differently, it is much more convenient for the > > > >> caller to > > > >> instantiate the device and have the fw_cfg default files already > > > >> preloaded > > > >> during realize. > > > >> > > > >> Move fw_cfg_init1() to the end of both the fw_cfg_mem_realize() and > > > >> fw_cfg_io_realize() functions so it no longer needs to be called > > > >> manually > > > >> when instantiating the device, and also rename it to > > > >> fw_cfg_common_realize() > > > >> which better describes its new purpose. > > > >> > > > >> Since it is now the responsibility of the machine to wire up the > > > >> fw_cfg device > > > >> it is necessary to introduce a object_property_add_child() call into > > > >> fw_cfg_init_io() and fw_cfg_init_mem() to link the fw_cfg device to > > > >> the root > > > >> machine object as before. > > > >> > > > >> Finally we can now convert the asserts() preventing multiple fw_cfg > > > >> devices > > > >> and unparented fw_cfg devices being instantiated and replace them with > > > >> proper > > > >> error reporting at realize time. This allows us to remove FW_CFG_NAME > > > >> and > > > >> FW_CFG_PATH since they are no longer required. > > > >> > > > >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> > > > >> --- > > > >> hw/nvram/fw_cfg.c | 41 +++++++++++++++++++++++++++++------------ > > > >> 1 file changed, 29 insertions(+), 12 deletions(-) > > > >> > > > >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > > > >> index 2291121..31029ac 100644 > > > >> --- a/hw/nvram/fw_cfg.c > > > >> +++ b/hw/nvram/fw_cfg.c > > > >> @@ -37,9 +37,6 @@ > > > >> > > > >> #define FW_CFG_FILE_SLOTS_DFLT 0x20 > > > >> > > > >> -#define FW_CFG_NAME "fw_cfg" > > > >> -#define FW_CFG_PATH "/machine/" FW_CFG_NAME > > > >> - > > > >> #define TYPE_FW_CFG "fw_cfg" > > > >> #define TYPE_FW_CFG_IO "fw_cfg_io" > > > >> #define TYPE_FW_CFG_MEM "fw_cfg_mem" > > > >> @@ -920,19 +917,22 @@ static int fw_cfg_unattached_at_realize(void) > > > >> } > > > >> > > > >> > > > >> -static void fw_cfg_init1(DeviceState *dev) > > > >> +static void fw_cfg_common_realize(DeviceState *dev, Error **errp) > > > >> { > > > >> FWCfgState *s = FW_CFG(dev); > > > >> MachineState *machine = MACHINE(qdev_get_machine()); > > > >> uint32_t version = FW_CFG_VERSION; > > > >> > > > >> - assert(!object_resolve_path(FW_CFG_PATH, NULL)); > > > >> - > > > >> - object_property_add_child(OBJECT(machine), FW_CFG_NAME, > > > >> OBJECT(s), NULL); > > > >> - > > > >> - qdev_init_nofail(dev); > > > >> + if (!fw_cfg_find()) { > > > > maybe add comment that here, that fw_cfg_find() will return NULL if > > > > more than > > > > 1 device is found. > > > > > > This should be the same behaviour as before, i.e. a NULL means that the > > > fw_cfg device couldn't be found. It seems intuitive to me from the name > > > of the function exactly what it does, so I don't find the lack of > > > comment too confusing. Does anyone else have any thoughts here? > > intuitive meaning from the function name would be return NULL if nothing > > were found, > > however it's not the case here. > > > > taking in account that fwcfg in not user creatable device how about: > > > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > > index 316fca9..8f6eef8 100644 > > --- a/hw/nvram/fw_cfg.c > > +++ b/hw/nvram/fw_cfg.c > > @@ -1014,7 +1014,10 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr > > data_addr) > > > > FWCfgState *fw_cfg_find(void) > > { > > - return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL)); > > + bool ambig = false; > > + FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, > > &ambig)); > > + assert(!ambig); > > + return f; > > } > > > > or if we must to print user friendly error and fail realize gracefully > > (not sure why) just add errp argument to function so it could report > > error back to caller, then places that do not care much about graceful > > exit would use error_abort as argument and realize would use > > its local_error/errp argument. > > I don't disagree with adding the assert(), but it looks like > making fw_cfg_find() return NULL if there are multiple devices > can be useful for realize. > > In this case, it looks like Mark is relying on that in > fw_cfg_common_realize(): if multiple devices are created, > fw_cfg_find() will return NULL, and realize will fail. This > sounds like a more graceful way to handle multiple-device > creation than crashing on fw_cfg_find(). This is the solution > used by find_vmgenid_dev()/vmgenid_realize(), BTW. I suspect that find_vmgenid_dev() works by luck as it could be placed only as /machine/peripheral-anon/foo1 or /machine/peripheral/foo2 object_resolve_partial_path() : machine object_resolve_partial_path() : peripheral-anon => foo1 object_resolve_partial_path() : peripheral => foo2 if (found /* foo2 */) { if (obj /* foo1 */) { return NULL; [...]