Re: [Qemu-devel] [PATCH 1/2] Make pointer to fw_cfg device global.

[Anthony Liguori <anthony@codemonkey.ws>]

On 05/23/2012 08:32 AM, Gleb Natapov wrote:
> On Wed, May 23, 2012 at 08:25:35AM -0500, Anthony Liguori wrote:
>> On 05/23/2012 07:37 AM, Gleb Natapov wrote:
>>> Ping.
>>
>> I don't understand why you're pinging..  The patch has just been on
>> the list for a couple of days and is definitely not a 1.1 candidate.
>> Am I missing something?
>>
> It is definitely not 1.1 candidate. Only 1.1 patches are accepted now?
> When master will be opened for 1.2 commits?

After 1.1 is released (June 1st).

>
>> However...
> Well, I am pinging for review :)
>
>>
>>>
>>> On Sun, May 20, 2012 at 12:02:40PM +0300, Gleb Natapov wrote:
>>>> There can be only one fw_cfg device, so saving global reference to it
>>>> removes the need to pass its pointer around.
>>>>
>>>> Signed-off-by: Gleb Natapov<gleb@redhat.com>
>>>> ---
>>>>   hw/fw_cfg.c       |  110 +++++++++++++++++++++++++++--------------------------
>>>>   hw/fw_cfg.h       |   15 +++----
>>>>   hw/loader.c       |   10 +----
>>>>   hw/loader.h       |    1 -
>>>>   hw/multiboot.c    |   17 ++++----
>>>>   hw/multiboot.h    |    3 +-
>>>>   hw/pc.c           |   63 ++++++++++++++----------------
>>>>   hw/ppc_newworld.c |   43 ++++++++++-----------
>>>>   hw/ppc_oldworld.c |   43 ++++++++++-----------
>>>>   hw/sun4m.c        |   93 +++++++++++++++++++++-----------------------
>>>>   hw/sun4u.c        |   35 ++++++++---------
>>>>   11 files changed, 207 insertions(+), 226 deletions(-)
>>>>
>>>> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
>>>> index 7b3b576..8c50473 100644
>>>> --- a/hw/fw_cfg.c
>>>> +++ b/hw/fw_cfg.c
>>>> @@ -48,7 +48,7 @@ typedef struct FWCfgEntry {
>>>>       FWCfgCallback callback;
>>>>   } FWCfgEntry;
>>>>
>>>> -struct FWCfgState {
>>>> +static struct FWCfgState {
>>>>       SysBusDevice busdev;
>>>>       MemoryRegion ctl_iomem, data_iomem, comb_iomem;
>>>>       uint32_t ctl_iobase, data_iobase;
>>>> @@ -57,7 +57,7 @@ struct FWCfgState {
>>>>       uint16_t cur_entry;
>>>>       uint32_t cur_offset;
>>>>       Notifier machine_ready;
>>>> -};
>>>> +} *fw_cfg;
>>>>
>>>>   #define JPG_FILE 0
>>>>   #define BMP_FILE 1
>>>> @@ -113,7 +113,7 @@ error:
>>>>       return NULL;
>>>>   }
>>>>
>>>> -static void fw_cfg_bootsplash(FWCfgState *s)
>>
>> This is a step backwards IMHO.  Relying on global state is generally
>> a bad thing.  Passing around pointers is a good thing because it
>> forces you to think about the relationships between devices and
>> makes it hard to do silly things (like have random interactions with
>> fw_cfg in devices that have no business doing that).
>>
> We are in a kind of agreement here, but fw_cfg is this rare thing that
> wants to be accessed by devices which, on a first glance, have no
> business doing that. We already saving fw_cfg globally for rom loaders
> to use, not nice either.

Rom loaders are an exception because they aren't being modeled as a device 
today.  For devices, we want the interactions to be expressed via the QOM graph 
which for now means having a PTR property (although actually on qom-next, it 
should be possible to do a proper link property).

Since you're interacting with fw_cfg in a proper device, you really should do so 
through the device interface (in this case, FWcfgState).

Regards,

Anthony Liguori