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

[Peter Maydell]

On Fri, 26 Jan 2024 at 15:20, David Woodhouse <dwmw2@infradead.org> 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.

> 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

Basically if the guest OS is entitled to assume the ethernet
device is present then we shouldn't allow the machine to be
created with it not present, because all that will happen is
that the guest will fall over in bootup.

(Similar applies to things like whether the board should
honour the option to disable USB support or not.)

thanks
-- PMM