Re: [Qemu-devel] [PATCH v2 1/5] fw_cfg: document fw_cfg_modify_iXX() update functions

[Gabriel L. Somlo]

On Tue, Sep 01, 2015 at 06:33:25PM +0100, Peter Maydell wrote:
> On 31 August 2015 at 10:10, Marc Marí <address@hidden> wrote:
> > From: "Gabriel L. Somlo" <address@hidden>
> >
> > Document the behavior of fw_cfg_modify_iXX() for leak-less updating
> > of integer-type blobs.
> >
> > Currently only fw_cfg_modify_i16() is coded, but 32- and 64-bit versions
> > may be added later if necessary..
> >
> > Signed-off-by: Gabriel Somlo <address@hidden>
> > Signed-off-by: Gerd Hoffmann <address@hidden>
> > Reviewed-by: Laszlo Ersek <address@hidden>
> > ---
> >  docs/specs/fw_cfg.txt | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> > index 74351dd..5bc7b96 100644
> > --- a/docs/specs/fw_cfg.txt
> > +++ b/docs/specs/fw_cfg.txt
> > @@ -159,6 +159,17 @@ will convert a 16-, 32-, or 64-bit integer to 
> > little-endian, then add
> >  a dynamically allocated copy of the appropriately sized item to fw_cfg
> >  under the given selector key value.
> >
> > +== fw_cfg_modify_iXX() ==
> > +
> > +Modify the value of an XX-bit item (where XX may be 16, 32, or 64).
> > +Similarly to the corresponding fw_cfg_add_iXX() function set, convert
> > +a 16-, 32-, or 64-bit integer to little endian, create a dynamically
> > +allocated copy of the required size, and replace the existing item at
> > +the given selector key value with the newly allocated one. The previous
> > +item, assumed to have been allocated during an earlier call to
> > +fw_cfg_add_iXX() or fw_cfg_modify_iXX() (of the same width XX), is freed
> > +before the function returns.
> > +
> >  == fw_cfg_add_file() ==
> >
> >  Given a filename (i.e., fw_cfg item name), starting pointer, and size,
> 
> This doesn't cover fw_cfg_modify_file(); is that intentional?

There should already be a paragraph in there (further down from where
this is supposed to go in) describing fw_cfg_modify_file(), which
isn't affected by this. 
> 
> As an aside, shouldn't this function-level documentation be done
> via doc-comments in the header file where the prototypes are
> declared? (You don't need to move the docs around in this series,
> but it might be nice to do at some point.)

You mean, leave docs/specs/fw_cfg.txt to deal with the guest-side
port/mmio api only, and have the host-side functions simply documented
as comments in include/hw/nvram/fw_cfg.h ?

That should be relatively painless, if that's the agreed-upon
convention...

Thanks,
--Gabriel