In the function external_snapshot_prepare() we have a
BlockdevSnapshotSync struct, which has the usual combination
of has_snapshot_node_name and snapshot_node_name fields for an
optional field. We set up a local variable
        const char *snapshot_node_name =
            s->has_snapshot_node_name ? s->snapshot_node_name : NULL;

and then mostly use "if (!snapshot_node_name)" for checking
whether we have a snapshot node name. The exception is that in
one place we check s->has_snapshot_node_name instead. This
confuses Coverity (CID 1396473), which thinks it might be
possible to get here with s->has_snapshot_node_name true but
snapshot_node_name NULL, and warns that the call to
qdict_put_str() will segfault in that case.

Make the code consistent and unconfuse Coverity by using
the same check for this conditional that we do in the rest
of the surrounding code.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Disclaimer: tested only with "make check"...

 blockdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 574adbcb7f5..b24610c606e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1639,7 +1639,7 @@ static void external_snapshot_prepare(BlkActionState *common,
         }
 
         options = qdict_new();
-        if (s->has_snapshot_node_name) {
+        if (snapshot_node_name) {
             qdict_put_str(options, "node-name", snapshot_node_name);
         }
         qdict_put_str(options, "driver", format);
-- 
2.19.1

On Thu 01 Nov 2018 05:30:37 PM CET, Peter Maydell wrote:
> In the function external_snapshot_prepare() we have a
> BlockdevSnapshotSync struct, which has the usual combination
> of has_snapshot_node_name and snapshot_node_name fields for an
> optional field. We set up a local variable
>         const char *snapshot_node_name =
>             s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
>
> and then mostly use "if (!snapshot_node_name)" for checking
> whether we have a snapshot node name. The exception is that in
> one place we check s->has_snapshot_node_name instead. This
> confuses Coverity (CID 1396473), which thinks it might be
> possible to get here with s->has_snapshot_node_name true but
> snapshot_node_name NULL, and warns that the call to
> qdict_put_str() will segfault in that case.
>
> Make the code consistent and unconfuse Coverity by using
> the same check for this conditional that we do in the rest
> of the surrounding code.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Am 01.11.2018 um 17:30 hat Peter Maydell geschrieben:
> In the function external_snapshot_prepare() we have a
> BlockdevSnapshotSync struct, which has the usual combination
> of has_snapshot_node_name and snapshot_node_name fields for an
> optional field. We set up a local variable
>         const char *snapshot_node_name =
>             s->has_snapshot_node_name ? s->snapshot_node_name : NULL;
> 
> and then mostly use "if (!snapshot_node_name)" for checking
> whether we have a snapshot node name. The exception is that in
> one place we check s->has_snapshot_node_name instead. This
> confuses Coverity (CID 1396473), which thinks it might be
> possible to get here with s->has_snapshot_node_name true but
> snapshot_node_name NULL, and warns that the call to
> qdict_put_str() will segfault in that case.
> 
> Make the code consistent and unconfuse Coverity by using
> the same check for this conditional that we do in the rest
> of the surrounding code.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Thanks, applied to the block branch.

Kevin