[PULL 0/2] fw_cfg patches for 2020-07-21

[PULL 0/2] fw_cfg patches for 2020-07-21

[Philippe Mathieu-Daudé]

The following changes since commit 90218a9a393c7925f330e7dcc08658e2a01d3bd4:

  Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2020-07=
-21' into staging (2020-07-21 10:24:38 +0100)

are available in the Git repository at:

  https://gitlab.com/philmd/qemu.git tags/fw_cfg-20200721

for you to fetch changes up to 077195187b47d83418e5fb521c89d7881fab3049:

  hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value (2020=
-07-21 16:47:54 +0200)

----------------------------------------------------------------
fw_cfg patches

Fixes the DEADCODE issue reported by Coverity (CID 1430396).

CI jobs result:
. https://gitlab.com/philmd/qemu/-/pipelines/169086301

----------------------------------------------------------------

Philippe Mathieu-Daud=C3=A9 (2):
  hw/nvram/fw_cfg: Simplify fw_cfg_add_from_generator() error
    propagation
  hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value

 include/hw/nvram/fw_cfg.h |  8 ++++++--
 hw/nvram/fw_cfg.c         | 13 +++++++------
 softmmu/vl.c              |  6 +-----
 3 files changed, 14 insertions(+), 13 deletions(-)

--=20
2.21.3

[PULL 1/2] hw/nvram/fw_cfg: Simplify fw_cfg_add_from_generator() error propagation

[Philippe Mathieu-Daudé]

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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Message-Id: <20200721131911.27380-2-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.
      */
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;
-- 
2.21.3

[PULL 2/2] hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value

[Philippe Mathieu-Daudé]

Commits b6d7e9b66f..a43770df5d simplified the error propagation.
Similarly to commit 6fd5bef10b "qom: Make functions taking Error**
return bool, not void", let fw_cfg_add_from_generator() return a
boolean value, not void.
This allow to simplify parse_fw_cfg() and fixes the error handling
issue reported by Coverity (CID 1430396):

  In parse_fw_cfg():

    Variable assigned once to a constant guards dead code.

    Local variable local_err is assigned only once, to a constant
    value, making it effectively constant throughout its scope.
    If this is not the intent, examine the logic to see if there
    is a missing assignment that would make local_err not remain
    constant.

It's the call of fw_cfg_add_from_generator():

        Error *local_err = NULL;

        fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
        if (local_err) {
            error_propagate(errp, local_err);
            return -1;
        }
        return 0;

If it fails, parse_fw_cfg() sets an error and returns 0, which is
wrong. Harmless, because the only caller passes &error_fatal.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Fixes: Coverity CID 1430396: 'Constant' variable guards dead code (DEADCODE)
Fixes: 6552d87c48 ("softmmu/vl: Let -fw_cfg option take a 'gen_id' argument")
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200721131911.27380-3-philmd@redhat.com>
---
 include/hw/nvram/fw_cfg.h |  4 +++-
 hw/nvram/fw_cfg.c         | 10 ++++++----
 softmmu/vl.c              |  6 +-----
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index bbcf405649..f190c428e8 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -304,8 +304,10 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename, void *data,
  * 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.
+ *
+ * Returns: %true on success, %false on error.
  */
-void fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
+bool fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
                                const char *gen_id, Error **errp);
 
 FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index dfa1f2012a..f3a4728288 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -1032,7 +1032,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
     return NULL;
 }
 
-void fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
+bool fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
                                const char *gen_id, Error **errp)
 {
     FWCfgDataGeneratorClass *klass;
@@ -1043,20 +1043,22 @@ void fw_cfg_add_from_generator(FWCfgState *s, const char *filename,
     obj = object_resolve_path_component(object_get_objects_root(), gen_id);
     if (!obj) {
         error_setg(errp, "Cannot find object ID '%s'", gen_id);
-        return;
+        return false;
     }
     if (!object_dynamic_cast(obj, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE)) {
         error_setg(errp, "Object ID '%s' is not a '%s' subclass",
                    gen_id, TYPE_FW_CFG_DATA_GENERATOR_INTERFACE);
-        return;
+        return false;
     }
     klass = FW_CFG_DATA_GENERATOR_GET_CLASS(obj);
     array = klass->get_data(obj, errp);
     if (!array) {
-        return;
+        return false;
     }
     size = array->len;
     fw_cfg_add_file(s, filename, g_byte_array_free(array, TRUE), size);
+
+    return true;
 }
 
 static void fw_cfg_machine_reset(void *opaque)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index f476ef89ed..3416241557 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2070,11 +2070,7 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp)
         size = strlen(str); /* NUL terminator NOT included in fw_cfg blob */
         buf = g_memdup(str, size);
     } else if (nonempty_str(gen_id)) {
-        Error *local_err = NULL;
-
-        fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (!fw_cfg_add_from_generator(fw_cfg, name, gen_id, errp)) {
             return -1;
         }
         return 0;
-- 
2.21.3

Re: [PULL 0/2] fw_cfg patches for 2020-07-21

[Peter Maydell]

On Tue, 21 Jul 2020 at 18:54, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> The following changes since commit 90218a9a393c7925f330e7dcc08658e2a01d3bd4:
>
>   Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2020-07=
> -21' into staging (2020-07-21 10:24:38 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/philmd/qemu.git tags/fw_cfg-20200721
>
> for you to fetch changes up to 077195187b47d83418e5fb521c89d7881fab3049:
>
>   hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value (2020=
> -07-21 16:47:54 +0200)
>
> ----------------------------------------------------------------
> fw_cfg patches
>
> Fixes the DEADCODE issue reported by Coverity (CID 1430396).
>
> CI jobs result:
> . https://gitlab.com/philmd/qemu/-/pipelines/169086301
>
> ----------------------------------------------------------------
>
> Philippe Mathieu-Daud=C3=A9 (2):

Something in your cover-letter creation is not handling UTF-8 right :-)

>   hw/nvram/fw_cfg: Simplify fw_cfg_add_from_generator() error
>     propagation
>   hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
for any user-visible changes.

-- PMM

Re: [PULL 0/2] fw_cfg patches for 2020-07-21

[Philippe Mathieu-Daudé]

On 7/23/20 12:43 PM, Peter Maydell wrote:
> On Tue, 21 Jul 2020 at 18:54, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> The following changes since commit 90218a9a393c7925f330e7dcc08658e2a01d3bd4:
>>
>>   Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2020-07=
>> -21' into staging (2020-07-21 10:24:38 +0100)
>>
>> are available in the Git repository at:
>>
>>   https://gitlab.com/philmd/qemu.git tags/fw_cfg-20200721
>>
>> for you to fetch changes up to 077195187b47d83418e5fb521c89d7881fab3049:
>>
>>   hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value (2020=
>> -07-21 16:47:54 +0200)
>>
>> ----------------------------------------------------------------
>> fw_cfg patches
>>
>> Fixes the DEADCODE issue reported by Coverity (CID 1430396).
>>
>> CI jobs result:
>> . https://gitlab.com/philmd/qemu/-/pipelines/169086301
>>
>> ----------------------------------------------------------------
>>
>> Philippe Mathieu-Daud=C3=A9 (2):
> 
> Something in your cover-letter creation is not handling UTF-8 right :-)

Yes, this is a known git-publish issue:
https://github.com/stefanha/git-publish/issues/88

It only happens with the cover, it doesn't affect the patches,
so it is considered a very low priority issue :)

> 
>>   hw/nvram/fw_cfg: Simplify fw_cfg_add_from_generator() error
>>     propagation
>>   hw/nvram/fw_cfg: Let fw_cfg_add_from_generator() return boolean value
> 
> 
> Applied, thanks.
> 
> Please update the changelog at https://wiki.qemu.org/ChangeLog/5.1
> for any user-visible changes.

Thanks (no user-visible changes in these patches).

Phil.