Re: [Qemu-block] [Qemu-devel] [PATCH 01/10] pflash: Rename pflash_t to PFlashCFI01, PFlashCFI02

[Philippe Mathieu-Daudé]

On 2/19/19 2:41 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <address@hidden> writes:
> 
>> On 2/18/19 1:56 PM, Markus Armbruster wrote:
>>> flash.h's incomplete struct pflash_t is completed both in
>>> pflash_cfi01.c and in pflash_cfi02.c.  The complete types are
>>> incompatible.  This can hide type errors, such as passing a pflash_t
>>> created with pflash_cfi02_register() to pflash_cfi01_get_memory().
>>>
>>> Furthermore, POSIX reserves typedef names ending with _t.

Worth adding in CODING_STYLE 'Naming' section :)

>>>
>>> Rename the two structs to PFlashCFI01 and PFlashCFI02.
>>
>> Why not ParallelFlashCFIxx?
> 
> Feels a bit long, and we abbreviate to pflash pretty consistently.  That
> said, I'm not particularly enamored with my choice of name :)
> 
>> Ideally ParallelFlashCFI would be an InterfaceInfo...
> 
> You mean TYPE_CFI_PFLASH0{1,2} should be children of an abstract parent?

I'd use "TYPE_PFLASH_CFI0[12]".

---

The "Common Flash memory Interface" as stated is definitively an
interface :)

QEMU models the 2 most famous industry implementations:

- vendor 0x01: Intel (and Sharp)
- vendor 0x02: AMD (and Fujistu)

---

My first refactor attempt was to have both implementations inheritate
from an abstract parent, but since the migration structures are
different, it looked easier to me to extract an InterfaceInfo.

>From here my idea was to add an NorFlash abstract parent where to share
the block devices and some of the reset:

            +-------------------+
            |                   |
            |    NOR Flash      |
            |                   |
            +-------------------+
              |    Parent     |
              |               |
              |               |
 +------------v---+       +---v------------+
 |                |       |                |
 |    PFlash01    |       |    PFlash02    |
 |                |       |                |
 +----------------+       +----------------+
     Child      |           |
                |           |
                | Implements|
            +---v-----------v---+
            |                   |
            |     CFI Flash     |
            |                   |
            +-------------------+

But since there is no consumer of the CFI InterfaceInfo, we can simply
go the way you suggested:

            +-------------------+
            |                   |
            |   CFI NOR Flash   |
            |                   |
            +-------------------+
              |    Parent     |
              |               |
 +------------v---+       +---v------------+
 |                |       |                |
 |    PFlash01    |       |    PFlash02    |
 |                |       |                |
 +----------------+       +----------------+
        Child                   Child