Re: [PATCH v2 1/2] hw/nvram/fw_cfg: Simplify fw_cfg_add_from_generator() error propagation

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Document FWCfgDataGeneratorClass::get_data() return NULL
> on error, and non-NULL on success. This allow us to simplify
> fw_cfg_add_from_generator(). Since we don't need a local
> variable to propagate the error, we can remove the ERRP_GUARD()
> macro.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/hw/nvram/fw_cfg.h | 4 +++-
>  hw/nvram/fw_cfg.c         | 3 +--
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index 11feae3177..bbcf405649 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -32,7 +32,9 @@ typedef struct FWCfgDataGeneratorClass {
>       * @obj: the object implementing this interface
>       * @errp: pointer to a NULL-initialized error object
>       *
> -     * Returns: reference to a byte array containing the data.
> +     * Returns: reference to a byte array containing the data on success,
> +     *          or NULL on error.
> +     *
>       * The caller should release the reference when no longer
>       * required.
>       */

Thanks for clarifying the doc comment!

As far as I can tell, the only implementation is
qcrypto_tls_cipher_suites_fw_cfg_gen_data(), which is a trivial wrapper
around qcrypto_tls_cipher_suites_get_data().  I checked it conforms.

> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 3b1811d3bf..dfa1f2012a 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -1035,7 +1035,6 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
> *filename,
>  void fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
>                                 const char *gen_id, Error **errp)
>  {
> -    ERRP_GUARD();
>      FWCfgDataGeneratorClass *klass;
>      GByteArray *array;
>      Object *obj;
> @@ -1053,7 +1052,7 @@ void fw_cfg_add_from_generator(FWCfgState *s, const 
> char *filename,
>      }
>      klass = FW_CFG_DATA_GENERATOR_GET_CLASS(obj);
>      array = klass->get_data(obj, errp);
> -    if (*errp) {
> +    if (!array) {
>          return;
>      }
>      size = array->len;

Reviewed-by: Markus Armbruster <armbru@redhat.com>