Allow the FW_CFG_DATA_GENERATOR interface get_data() handler to
return NULL when there is nothing to generate. In that case
fw_cfg_add_file_from_generator() will not add any item and
return %true.

Reported-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/nvram/fw_cfg.h | 13 ++++++++-----
 hw/nvram/fw_cfg.c         | 10 ++++++----
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index fcb06f18cc3..5211018fd8f 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -30,8 +30,9 @@ 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 on success,
-     *          or NULL on error.
+     * Returns: NULL on failure (errp set if not NULL).
+     *          A byte array containing the data (if any,
+     *          otherwise NULL) on success.
      *
      * The caller should release the reference when no longer
      * required.
@@ -298,14 +299,16 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
  * @parent: the object in which to resolve the @part
  * @errp: pointer to a NULL initialized error object
  *
- * Add a new NAMED fw_cfg item with the content generated from the
- * @part object. The data generated by the @part object is copied
- * into the data structure of the fw_cfg device.
+ * If the @part object generates content, add a new NAMED fw_cfg item with it.
+ * The data generated by the @part object is copied into the data structure of
+ * the fw_cfg device.
  * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
  * will be used; also, a new entry will be added to the file directory
  * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
  * data size, and assigned selector key value.
  *
+ * If the @part object does not generate content, no fw_cfg item is added.
+ *
  * Returns: %true on success, %false on error.
  */
 bool fw_cfg_add_file_from_generator(FWCfgState *s,
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index b94cd27bd85..7e1065e5f50 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1031,10 +1031,10 @@ bool fw_cfg_add_file_from_generator(FWCfgState *s,
                                     Object *parent, const char *part,
                                     const char *filename, Error **errp)
 {
+    ERRP_GUARD();
     FWCfgDataGeneratorClass *klass;
     GByteArray *array;
     Object *obj;
-    gsize size;
 
     obj = object_resolve_path_component(parent, part);
     if (!obj) {
@@ -1048,11 +1048,13 @@ bool fw_cfg_add_file_from_generator(FWCfgState *s,
     }
     klass = FW_CFG_DATA_GENERATOR_GET_CLASS(obj);
     array = klass->get_data(obj, errp);
-    if (!array) {
+    if (*errp) {
         return false;
     }
-    size = array->len;
-    fw_cfg_add_file(s, filename, g_byte_array_free(array, FALSE), size);
+    if (array) {
+        fw_cfg_add_file(s, filename, g_byte_array_free(array, FALSE),
+                        array->len);
+    }
 
     return true;
 }
-- 
2.45.2

On Fri, Dec 13, 2024 at 02:33:48PM +0100, Philippe Mathieu-Daudé wrote:
> Allow the FW_CFG_DATA_GENERATOR interface get_data() handler to
> return NULL when there is nothing to generate. In that case
> fw_cfg_add_file_from_generator() will not add any item and
> return %true.
> 
> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/nvram/fw_cfg.h | 13 ++++++++-----
>  hw/nvram/fw_cfg.c         | 10 ++++++----
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index fcb06f18cc3..5211018fd8f 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -30,8 +30,9 @@ 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 on success,
> -     *          or NULL on error.
> +     * Returns: NULL on failure (errp set if not NULL).
> +     *          A byte array containing the data (if any,
> +     *          otherwise NULL) on success.

Bit confusing wording, lets say

  Returns: A byte array containing data to add, or NULL without
           @errp set if no data is required, or NULL with @errp
	   set on failure.

With that change:

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 13/12/24 14:48, Daniel P. Berrangé wrote:
> On Fri, Dec 13, 2024 at 02:33:48PM +0100, Philippe Mathieu-Daudé wrote:
>> Allow the FW_CFG_DATA_GENERATOR interface get_data() handler to
>> return NULL when there is nothing to generate. In that case
>> fw_cfg_add_file_from_generator() will not add any item and
>> return %true.
>>
>> Reported-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/nvram/fw_cfg.h | 13 ++++++++-----
>>   hw/nvram/fw_cfg.c         | 10 ++++++----
>>   2 files changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
>> index fcb06f18cc3..5211018fd8f 100644
>> --- a/include/hw/nvram/fw_cfg.h
>> +++ b/include/hw/nvram/fw_cfg.h
>> @@ -30,8 +30,9 @@ 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 on success,
>> -     *          or NULL on error.
>> +     * Returns: NULL on failure (errp set if not NULL).
>> +     *          A byte array containing the data (if any,
>> +     *          otherwise NULL) on success.
> 
> Bit confusing wording, lets say
> 
>    Returns: A byte array containing data to add, or NULL without
>             @errp set if no data is required, or NULL with @errp
> 	   set on failure.

Thank you!

> 
> With that change:
> 
>    Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> With regards,
> Daniel