Re: [PATCH v3 01/46] net: add qemu_{configure,create}_nic_device(), qemu_find_nic_info()

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Fri, 2024-01-26 at 15:33 +0000, Peter Maydell wrote:
> On Fri, 26 Jan 2024 at 15:20, David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote:
> > 
> > On Fri, 2024-01-26 at 14:43 +0000, Peter Maydell wrote:
> > > 
> > > > +NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
> > > > +                            const char *alias);
> > > > +bool qemu_configure_nic_device(DeviceState *dev, bool match_default,
> > > > +                               const char *alias);
> > > > +DeviceState *qemu_create_nic_device(const char *typename, bool 
> > > > match_default,
> > > > +                                    const char *alias);
> > > 
> > > Could we have doc comments that document the purpose and API
> > > for these new global functions, please?
> > 
> > Like this? I deliberately fatfingered the argument names and didn't
> > even get a build warning, and I don't see any actual *documentation*
> > being generated with it...?
> 
> We use the doc comment format to allow for potential future
> documentation generation, but it's only actually generated
> if there's a .rst file somewhere under docs/ that has a
> kernel-doc:: directive referencing the .h file (for instance
> there's one in docs/devel/memory.rst that results in
> https://www.qemu.org/docs/master/devel/memory.html#api-reference ;)
> 
> For almost all internal functions, we set the relatively low
> bar of "have a doc comment so people reading the header file
> can see what the functions do". Where there's a more complex
> subsystem that merits its own hand-written documentation
> under docs/devel, then if the author of that documentation
> is enthusiastic they can clean up and pull in specific headers
> to add autogenerated docs. But the primary audience is the
> human reader of the .h file.

Ack, thanks.

> > diff --git a/include/net/net.h b/include/net/net.h
> > index 25ea83fd12..14614b0a31 100644
> > --- a/include/net/net.h
> > +++ b/include/net/net.h
> > @@ -207,10 +207,46 @@ int qemu_show_nic_models(const char *arg, const char 
> > *const *models);
> >  void qemu_check_nic_model(NICInfo *nd, const char *model);
> >  int qemu_find_nic_model(NICInfo *nd, const char * const *models,
> >                          const char *default_model);
> > +/**
> > + * qemu_find_nic_info: Obtain NIC configuration information
> > + * @typename: Name of device object type
> > + * @match_default: Match NIC configurations with no model specified
> > + * @alias: Additional model string to match (for user convenience and
> > + *         backward compatibility).
> > + *
> > + * Search for a NIC configuration matching the NIC model constraints.
> > + */
> >  NICInfo *qemu_find_nic_info(const char *typename, bool match_default,
> >                              const char *alias);
> > +/**
> > + * qemu_configure_nic_device: Apply NIC configuration to a given device
> > + * @dev: Network device to be configured
> > + * @match_default: Match NIC configurations with no model specified
> > + * @alias: Additional model string to match
> > + *
> > + * Search for a NIC configuration for the provided device, using the
> > + * additionally specified matching constraints. If found, apply the
> > + * configuration using qdev_set_nic_properties() and return %true.
> > + *
> > + * This is used by platform code which creates the device anyway,
> > + * regardless of whether there is a configuration for it. This tends
> > + * to be platforms which ignore `--nodefaults` and create net devices
> > + * anyway. This behaviour is not advised for new platforms; use the
> > + * qemu_create_nic_device() function instead, which creates the device
> > + * only when it is configured.
> 
> I disagree about this paragraph. The behaviour we want for new
> platforms is:
> 
>  * If this is modelling some board where the ethernet device is
>    always present (eg it is soldered on to the board, or it is
>    a part of the SoC that the board uses), then always create
>    that device
>  * If the hardware being modelled has the ethernet device as an
>    optional device (eg physically removable like a PCI card),
>    then the board should arrange that --nodefaults causes it to
>    not be created
> 

Ack. Scratch the 'better behaved' part of my last response to Thomas
about smc91c111 too then :)

How's this:

/**
 * qemu_configure_nic_device: Apply NIC configuration to a given device
 * @dev: Network device to be configured
 * @match_default: Match NIC configurations with no model specified
 * @alias: Additional model string to match
 *
 * Search for a NIC configuration for the provided device, using the
 * additionally specified matching constraints. If found, apply the
 * configuration using qdev_set_nic_properties() and return %true.
 *
 * This is used by platform code which creates the device anyway,
 * regardless of whether there is a configuration for it. This tends
 * to be platforms which ignore `--nodefaults` and create net devices
 * anyway, for example because the Ethernet device on that board is
 * always physically present.
 */

Attachment: smime.p7s
Description: S/MIME cryptographic signature