| Message ID | 1738788841-211843-1-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | migration: use parameters.mode in cpr_state_save | expand |
On Wed, Feb 05, 2025 at 12:54:01PM -0800, Steve Sistare wrote: > qmp_migrate guarantees that cpr_channel is not null for > MIG_MODE_CPR_TRANSFER when cpr_state_save is called: > > qmp_migrate() > if (s->parameters.mode == MIG_MODE_CPR_TRANSFER && !cpr_channel) { > return; > } > cpr_state_save(cpr_channel) > > but cpr_state_save checks for mode differently before using channel, > and Coverity cannot infer that they are equivalent in outgoing QEMU, > and warns that channel may be NULL: > > cpr_state_save(channel) > MigMode mode = migrate_mode(); > if (mode == MIG_MODE_CPR_TRANSFER) { > f = cpr_transfer_output(channel, errp); > > To make Coverity happy, use parameters.mode in cpr_state_save. > > Resolves: Coverity CID 1590980 > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > migration/cpr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/migration/cpr.c b/migration/cpr.c > index 584b0b9..7f20bd5 100644 > --- a/migration/cpr.c > +++ b/migration/cpr.c > @@ -8,6 +8,7 @@ > #include "qemu/osdep.h" > #include "qapi/error.h" > #include "migration/cpr.h" > +#include "migration/migration.h" > #include "migration/misc.h" > #include "migration/options.h" > #include "migration/qemu-file.h" > @@ -132,7 +133,7 @@ int cpr_state_save(MigrationChannel *channel, Error **errp) > { > int ret; > QEMUFile *f; > - MigMode mode = migrate_mode(); > + MigMode mode = migrate_get_current()->parameters.mode; Are we sure this can make coverity happy? Another more straightforward change is caching migrate mode in qmp_migrate() and also check that before invoking cpr_state_save(). Thanks, > > trace_cpr_state_save(MigMode_str(mode)); > > -- > 1.8.3.1 >
On 2/5/2025 4:28 PM, Peter Xu wrote: > On Wed, Feb 05, 2025 at 12:54:01PM -0800, Steve Sistare wrote: >> qmp_migrate guarantees that cpr_channel is not null for >> MIG_MODE_CPR_TRANSFER when cpr_state_save is called: >> >> qmp_migrate() >> if (s->parameters.mode == MIG_MODE_CPR_TRANSFER && !cpr_channel) { >> return; >> } >> cpr_state_save(cpr_channel) >> >> but cpr_state_save checks for mode differently before using channel, >> and Coverity cannot infer that they are equivalent in outgoing QEMU, >> and warns that channel may be NULL: >> >> cpr_state_save(channel) >> MigMode mode = migrate_mode(); >> if (mode == MIG_MODE_CPR_TRANSFER) { >> f = cpr_transfer_output(channel, errp); >> >> To make Coverity happy, use parameters.mode in cpr_state_save. >> >> Resolves: Coverity CID 1590980 >> Reported-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> migration/cpr.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/migration/cpr.c b/migration/cpr.c >> index 584b0b9..7f20bd5 100644 >> --- a/migration/cpr.c >> +++ b/migration/cpr.c >> @@ -8,6 +8,7 @@ >> #include "qemu/osdep.h" >> #include "qapi/error.h" >> #include "migration/cpr.h" >> +#include "migration/migration.h" >> #include "migration/misc.h" >> #include "migration/options.h" >> #include "migration/qemu-file.h" >> @@ -132,7 +133,7 @@ int cpr_state_save(MigrationChannel *channel, Error **errp) >> { >> int ret; >> QEMUFile *f; >> - MigMode mode = migrate_mode(); >> + MigMode mode = migrate_get_current()->parameters.mode; > > Are we sure this can make coverity happy? It should, based on Peter Maydell's analysis, but I would appreciate if he could apply and test the fix. > Another more straightforward change is caching migrate mode in > qmp_migrate() and also check that before invoking cpr_state_save(). Surely anyone would consider my one-line change to be straight forward. - Steve >> trace_cpr_state_save(MigMode_str(mode)); >> >> -- >> 1.8.3.1 >> >
On 2/5/2025 4:52 PM, Steven Sistare wrote: > On 2/5/2025 4:28 PM, Peter Xu wrote: >> On Wed, Feb 05, 2025 at 12:54:01PM -0800, Steve Sistare wrote: >>> qmp_migrate guarantees that cpr_channel is not null for >>> MIG_MODE_CPR_TRANSFER when cpr_state_save is called: >>> >>> qmp_migrate() >>> if (s->parameters.mode == MIG_MODE_CPR_TRANSFER && !cpr_channel) { >>> return; >>> } >>> cpr_state_save(cpr_channel) >>> >>> but cpr_state_save checks for mode differently before using channel, >>> and Coverity cannot infer that they are equivalent in outgoing QEMU, >>> and warns that channel may be NULL: >>> >>> cpr_state_save(channel) >>> MigMode mode = migrate_mode(); >>> if (mode == MIG_MODE_CPR_TRANSFER) { >>> f = cpr_transfer_output(channel, errp); >>> >>> To make Coverity happy, use parameters.mode in cpr_state_save. >>> >>> Resolves: Coverity CID 1590980 >>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >>> --- >>> migration/cpr.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/migration/cpr.c b/migration/cpr.c >>> index 584b0b9..7f20bd5 100644 >>> --- a/migration/cpr.c >>> +++ b/migration/cpr.c >>> @@ -8,6 +8,7 @@ >>> #include "qemu/osdep.h" >>> #include "qapi/error.h" >>> #include "migration/cpr.h" >>> +#include "migration/migration.h" >>> #include "migration/misc.h" >>> #include "migration/options.h" >>> #include "migration/qemu-file.h" >>> @@ -132,7 +133,7 @@ int cpr_state_save(MigrationChannel *channel, Error **errp) >>> { >>> int ret; >>> QEMUFile *f; >>> - MigMode mode = migrate_mode(); >>> + MigMode mode = migrate_get_current()->parameters.mode; >> >> Are we sure this can make coverity happy? > > It should, based on Peter Maydell's analysis, but I would appreciate > if he could apply and test the fix. > >> Another more straightforward change is caching migrate mode in >> qmp_migrate() and also check that before invoking cpr_state_save(). > > Surely anyone would consider my one-line change to be straight forward. Given that Coverity complains about channel, and not mode, this is the most direct fix: ---------------------------------------- diff --git a/migration/cpr.c b/migration/cpr.c index 59644e8..224b6ff 100644 --- a/migration/cpr.c +++ b/migration/cpr.c @@ -160,6 +160,7 @@ int cpr_state_save(MigrationChannel *channel, Error **errp) trace_cpr_state_save(MigMode_str(mode)); if (mode == MIG_MODE_CPR_TRANSFER) { + g_assert(channel); f = cpr_transfer_output(channel, errp); } else { return 0; ------------------------------- - Steve
Steven Sistare <steven.sistare@oracle.com> writes: > On 2/5/2025 4:52 PM, Steven Sistare wrote: >> On 2/5/2025 4:28 PM, Peter Xu wrote: >>> On Wed, Feb 05, 2025 at 12:54:01PM -0800, Steve Sistare wrote: >>>> qmp_migrate guarantees that cpr_channel is not null for >>>> MIG_MODE_CPR_TRANSFER when cpr_state_save is called: >>>> >>>> qmp_migrate() >>>> if (s->parameters.mode == MIG_MODE_CPR_TRANSFER && !cpr_channel) { >>>> return; >>>> } >>>> cpr_state_save(cpr_channel) >>>> >>>> but cpr_state_save checks for mode differently before using channel, >>>> and Coverity cannot infer that they are equivalent in outgoing QEMU, >>>> and warns that channel may be NULL: >>>> >>>> cpr_state_save(channel) >>>> MigMode mode = migrate_mode(); >>>> if (mode == MIG_MODE_CPR_TRANSFER) { >>>> f = cpr_transfer_output(channel, errp); >>>> >>>> To make Coverity happy, use parameters.mode in cpr_state_save. >>>> >>>> Resolves: Coverity CID 1590980 >>>> Reported-by: Peter Maydell <peter.maydell@linaro.org> >>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >>>> --- >>>> migration/cpr.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/migration/cpr.c b/migration/cpr.c >>>> index 584b0b9..7f20bd5 100644 >>>> --- a/migration/cpr.c >>>> +++ b/migration/cpr.c >>>> @@ -8,6 +8,7 @@ >>>> #include "qemu/osdep.h" >>>> #include "qapi/error.h" >>>> #include "migration/cpr.h" >>>> +#include "migration/migration.h" >>>> #include "migration/misc.h" >>>> #include "migration/options.h" >>>> #include "migration/qemu-file.h" >>>> @@ -132,7 +133,7 @@ int cpr_state_save(MigrationChannel *channel, Error **errp) >>>> { >>>> int ret; >>>> QEMUFile *f; >>>> - MigMode mode = migrate_mode(); >>>> + MigMode mode = migrate_get_current()->parameters.mode; >>> >>> Are we sure this can make coverity happy? >> >> It should, based on Peter Maydell's analysis, but I would appreciate >> if he could apply and test the fix. >> >>> Another more straightforward change is caching migrate mode in >>> qmp_migrate() and also check that before invoking cpr_state_save(). >> >> Surely anyone would consider my one-line change to be straight forward. > > > Given that Coverity complains about channel, and not mode, this is the > most direct fix: > > ---------------------------------------- > diff --git a/migration/cpr.c b/migration/cpr.c > index 59644e8..224b6ff 100644 > --- a/migration/cpr.c > +++ b/migration/cpr.c > @@ -160,6 +160,7 @@ int cpr_state_save(MigrationChannel *channel, Error **errp) > trace_cpr_state_save(MigMode_str(mode)); > > if (mode == MIG_MODE_CPR_TRANSFER) { > + g_assert(channel); > f = cpr_transfer_output(channel, errp); > } else { > return 0; > ------------------------------- > > - Steve Queueing this^ version. Thanks
diff --git a/migration/cpr.c b/migration/cpr.c index 584b0b9..7f20bd5 100644 --- a/migration/cpr.c +++ b/migration/cpr.c @@ -8,6 +8,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "migration/cpr.h" +#include "migration/migration.h" #include "migration/misc.h" #include "migration/options.h" #include "migration/qemu-file.h" @@ -132,7 +133,7 @@ int cpr_state_save(MigrationChannel *channel, Error **errp) { int ret; QEMUFile *f; - MigMode mode = migrate_mode(); + MigMode mode = migrate_get_current()->parameters.mode; trace_cpr_state_save(MigMode_str(mode));
qmp_migrate guarantees that cpr_channel is not null for MIG_MODE_CPR_TRANSFER when cpr_state_save is called: qmp_migrate() if (s->parameters.mode == MIG_MODE_CPR_TRANSFER && !cpr_channel) { return; } cpr_state_save(cpr_channel) but cpr_state_save checks for mode differently before using channel, and Coverity cannot infer that they are equivalent in outgoing QEMU, and warns that channel may be NULL: cpr_state_save(channel) MigMode mode = migrate_mode(); if (mode == MIG_MODE_CPR_TRANSFER) { f = cpr_transfer_output(channel, errp); To make Coverity happy, use parameters.mode in cpr_state_save. Resolves: Coverity CID 1590980 Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- migration/cpr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)