| Message ID | 20180926094219.20322-9-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory-device: complete refactoring + virtio-pmem | expand |
On Wed, 26 Sep 2018 11:42:03 +0200 David Hildenbrand <david@redhat.com> wrote: > Document the functions and when to not expect errors. > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: David Hildenbrand <david@redhat.com> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > include/hw/mem/memory-device.h | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h > index f02b229837..1d15cfc357 100644 > --- a/include/hw/mem/memory-device.h > +++ b/include/hw/mem/memory-device.h > @@ -29,9 +29,25 @@ typedef struct MemoryDeviceState { > Object parent_obj; > } MemoryDeviceState; > > +/** > + * MemoryDeviceClass: > + * @get_addr: The address of the @md in guest physical memory. "0" means that > + * no address has been specified by the user and that no address has been > + * assigned yet. > + * @get_plugged_size: The amount of memory provided by this @md currently > + * usable ("plugged") by the guest. This is helpful for devices that > + * dynamically manage the amount of memory accessible by the guest via > + * the reserved memory region. For most devices, this corresponds to the > + * size of the memory region. > + * @get_region_size: The size of the memory region of the @md that's mapped > + * in guest physical memory at @get_addr. > + * @fill_device_info: Translate current @md state into #MemoryDeviceInfo. > + */ > typedef struct MemoryDeviceClass { > + /* private */ > InterfaceClass parent_class; > > + /* public */ > uint64_t (*get_addr)(const MemoryDeviceState *md); > uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp); > uint64_t (*get_region_size)(const MemoryDeviceState *md, Error **errp);
Hi David, On 9/26/18 11:42 AM, David Hildenbrand wrote: > Document the functions and when to not expect errors. > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > include/hw/mem/memory-device.h | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h > index f02b229837..1d15cfc357 100644 > --- a/include/hw/mem/memory-device.h > +++ b/include/hw/mem/memory-device.h > @@ -29,9 +29,25 @@ typedef struct MemoryDeviceState { > Object parent_obj; > } MemoryDeviceState; > > +/**> + * MemoryDeviceClass: > + * @get_addr: The address of the @md in guest physical memory. "0" means that > + * no address has been specified by the user and that no address has been > + * assigned yet. > + * @get_plugged_size: The amount of memory provided by this @md currently > + * usable ("plugged") by the guest. This is helpful for devices that > + * dynamically manage the amount of memory accessible by the guest via > + * the reserved memory region. For most devices, this corresponds to the > + * size of the memory region. > + * @get_region_size: The size of the memory region of the @md that's mapped > + * in guest physical memory at @get_addr. > + * @fill_device_info: Translate current @md state into #MemoryDeviceInfo. I think it may be relevant to document whether all those functions are mandated or if any are optional. With respect to the form, you could get inspired of include/exec/memory.h (see for instance IOMMUMemoryRegionClass doc) Thanks Eric > + */ > typedef struct MemoryDeviceClass { > + /* private */ > InterfaceClass parent_class; > > + /* public */ > uint64_t (*get_addr)(const MemoryDeviceState *md); > uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp); > uint64_t (*get_region_size)(const MemoryDeviceState *md, Error **errp); >
On 30/09/2018 16:43, Auger Eric wrote: > Hi David, > > On 9/26/18 11:42 AM, David Hildenbrand wrote: >> Document the functions and when to not expect errors. >> >> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- >> include/hw/mem/memory-device.h | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h >> index f02b229837..1d15cfc357 100644 >> --- a/include/hw/mem/memory-device.h >> +++ b/include/hw/mem/memory-device.h >> @@ -29,9 +29,25 @@ typedef struct MemoryDeviceState { >> Object parent_obj; >> } MemoryDeviceState; >> >> +/**> + * MemoryDeviceClass: >> + * @get_addr: The address of the @md in guest physical memory. "0" means that >> + * no address has been specified by the user and that no address has been >> + * assigned yet. >> + * @get_plugged_size: The amount of memory provided by this @md currently >> + * usable ("plugged") by the guest. This is helpful for devices that >> + * dynamically manage the amount of memory accessible by the guest via >> + * the reserved memory region. For most devices, this corresponds to the >> + * size of the memory region. >> + * @get_region_size: The size of the memory region of the @md that's mapped >> + * in guest physical memory at @get_addr. >> + * @fill_device_info: Translate current @md state into #MemoryDeviceInfo. > I think it may be relevant to document whether all those functions are > mandated or if any are optional. All are mandatory if not otherwise specified (right now none) .... > > With respect to the form, you could get inspired of > include/exec/memory.h (see for instance IOMMUMemoryRegionClass doc) > ... which seems to be the same approach used in IOMMUMemoryRegionClass. While I am a friend of documentation, I prefer to keep it short where possible. (e.g. obvious things like "@md: the MemoryDeviceState", I don't consider useful) I am using right now a similar style as in include/hw/mem/pc-dimm.h and include/exec/memory.h. BTW, is there a coding style about such things? (where, how to document classes, structs, fuucntions and parameters) Thanks!
Hi David, On 10/1/18 10:13 AM, David Hildenbrand wrote: > On 30/09/2018 16:43, Auger Eric wrote: >> Hi David, >> >> On 9/26/18 11:42 AM, David Hildenbrand wrote: >>> Document the functions and when to not expect errors. >>> >>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >> >>> --- >>> include/hw/mem/memory-device.h | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h >>> index f02b229837..1d15cfc357 100644 >>> --- a/include/hw/mem/memory-device.h >>> +++ b/include/hw/mem/memory-device.h >>> @@ -29,9 +29,25 @@ typedef struct MemoryDeviceState { >>> Object parent_obj; >>> } MemoryDeviceState; >>> >>> +/**> + * MemoryDeviceClass: >>> + * @get_addr: The address of the @md in guest physical memory. "0" means that >>> + * no address has been specified by the user and that no address has been >>> + * assigned yet. >>> + * @get_plugged_size: The amount of memory provided by this @md currently >>> + * usable ("plugged") by the guest. This is helpful for devices that >>> + * dynamically manage the amount of memory accessible by the guest via >>> + * the reserved memory region. For most devices, this corresponds to the >>> + * size of the memory region. >>> + * @get_region_size: The size of the memory region of the @md that's mapped >>> + * in guest physical memory at @get_addr. >>> + * @fill_device_info: Translate current @md state into #MemoryDeviceInfo. >> I think it may be relevant to document whether all those functions are >> mandated or if any are optional. > > All are mandatory if not otherwise specified (right now none) .... > >> >> With respect to the form, you could get inspired of >> include/exec/memory.h (see for instance IOMMUMemoryRegionClass doc) >> > > ... which seems to be the same approach used in IOMMUMemoryRegionClass. > > While I am a friend of documentation, I prefer to keep it short where > possible. (e.g. obvious things like "@md: the MemoryDeviceState", I > don't consider useful) agreed ;-) > > I am using right now a similar style as in include/hw/mem/pc-dimm.h and > include/exec/memory.h. > > BTW, is there a coding style about such things? (where, how to document > classes, structs, fuucntions and parameters) I am not aware of any. IOMMUMemoryRegionClass doc was written latterly by Peter so I would be tempted to think this is a good model ;-) Thanks Eric > > Thanks! > >
diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h index f02b229837..1d15cfc357 100644 --- a/include/hw/mem/memory-device.h +++ b/include/hw/mem/memory-device.h @@ -29,9 +29,25 @@ typedef struct MemoryDeviceState { Object parent_obj; } MemoryDeviceState; +/** + * MemoryDeviceClass: + * @get_addr: The address of the @md in guest physical memory. "0" means that + * no address has been specified by the user and that no address has been + * assigned yet. + * @get_plugged_size: The amount of memory provided by this @md currently + * usable ("plugged") by the guest. This is helpful for devices that + * dynamically manage the amount of memory accessible by the guest via + * the reserved memory region. For most devices, this corresponds to the + * size of the memory region. + * @get_region_size: The size of the memory region of the @md that's mapped + * in guest physical memory at @get_addr. + * @fill_device_info: Translate current @md state into #MemoryDeviceInfo. + */ typedef struct MemoryDeviceClass { + /* private */ InterfaceClass parent_class; + /* public */ uint64_t (*get_addr)(const MemoryDeviceState *md); uint64_t (*get_plugged_size)(const MemoryDeviceState *md, Error **errp); uint64_t (*get_region_size)(const MemoryDeviceState *md, Error **errp);