hw/nvme/ctrl.c | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-)
From: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
Let's use GPtrArray to build a list of blocker features and then
g_strjoinv() to build a final comma-delimited string.
While previous approach was technically correct, it is fragile
(because we need to take care of static buffer size choice) and
Coverity dislikes it too.
Note, that we use g_ptr_array_new() to allocate array which means
that GDestroyNotify callback is not set, so we can pass pointers to
a static memory like g_ptr_array_add(..., (gpointer) "SR-IOV") without
any problems as there won't be any attempt to free that memory.
Resolves: Coverity CID 1663673
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
---
hw/nvme/ctrl.c | 41 +++++++++++++++++------------------------
1 file changed, 17 insertions(+), 24 deletions(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index a67e1598891..95a586d82a9 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -9352,22 +9352,11 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
}
}
-#define BLOCKER_FEATURES_MAX_LEN 256
-
-static inline void nvme_add_blocker_feature(char *blocker_features,
- const char *feature)
-{
- if (strlen(blocker_features) > 0) {
- g_strlcat(blocker_features, ", ", BLOCKER_FEATURES_MAX_LEN);
- }
- g_strlcat(blocker_features, feature, BLOCKER_FEATURES_MAX_LEN);
-}
-
static bool nvme_set_migration_blockers(NvmeCtrl *n, PCIDevice *pci_dev,
Error **errp)
{
uint64_t unsupported_cap, cap = ldq_le_p(&n->bar.cap);
- char blocker_features[BLOCKER_FEATURES_MAX_LEN] = "";
+ g_autoptr(GPtrArray) blocker_features = g_ptr_array_new();
bool adm_cmd_security_checked = false;
bool cmd_io_mgmt_checked = false;
bool cmd_zone_checked = false;
@@ -9416,15 +9405,15 @@ static bool nvme_set_migration_blockers(NvmeCtrl *n, PCIDevice *pci_dev,
}
if (namespaces_num > 1) {
- nvme_add_blocker_feature(blocker_features,
- "Namespace Attachment");
+ g_ptr_array_add(blocker_features,
+ (gpointer) "Namespace Attachment");
}
break;
}
case NVME_ADM_CMD_VIRT_MNGMT:
if (n->params.sriov_max_vfs) {
- nvme_add_blocker_feature(blocker_features, "SR-IOV");
+ g_ptr_array_add(blocker_features, (gpointer) "SR-IOV");
}
break;
@@ -9435,7 +9424,7 @@ static bool nvme_set_migration_blockers(NvmeCtrl *n, PCIDevice *pci_dev,
}
if (pci_dev->spdm_port) {
- nvme_add_blocker_feature(blocker_features, "SPDM");
+ g_ptr_array_add(blocker_features, (gpointer) "SPDM");
}
adm_cmd_security_checked = true;
@@ -9469,7 +9458,7 @@ static bool nvme_set_migration_blockers(NvmeCtrl *n, PCIDevice *pci_dev,
/* check for NVME_IOMS_MO_RUH_UPDATE */
if (n->subsys->params.fdp.enabled) {
- nvme_add_blocker_feature(blocker_features, "FDP");
+ g_ptr_array_add(blocker_features, (gpointer) "FDP");
}
cmd_io_mgmt_checked = true;
@@ -9504,8 +9493,8 @@ static bool nvme_set_migration_blockers(NvmeCtrl *n, PCIDevice *pci_dev,
}
if (ns->params.zoned) {
- nvme_add_blocker_feature(blocker_features,
- "Zoned Namespace");
+ g_ptr_array_add(blocker_features,
+ (gpointer) "Zoned Namespace");
break;
}
}
@@ -9525,24 +9514,28 @@ static bool nvme_set_migration_blockers(NvmeCtrl *n, PCIDevice *pci_dev,
* covered by unsupported_cap check.
*/
if (NVME_CAP_CMBS(cap)) {
- nvme_add_blocker_feature(blocker_features, "CMB");
+ g_ptr_array_add(blocker_features, (gpointer) "CMB");
cap &= ~((uint64_t)CAP_CMBS_MASK << CAP_CMBS_SHIFT);
}
if (NVME_CAP_PMRS(cap)) {
- nvme_add_blocker_feature(blocker_features, "PMR");
+ g_ptr_array_add(blocker_features, (gpointer) "PMR");
cap &= ~((uint64_t)CAP_PMRS_MASK << CAP_PMRS_SHIFT);
}
unsupported_cap = cap & ~NVME_MIGRATION_SUPPORTED_CAP_BITS;
if (unsupported_cap) {
- nvme_add_blocker_feature(blocker_features, "unknown capability");
+ g_ptr_array_add(blocker_features, (gpointer) "unknown capability");
}
assert(n->migration_blocker == NULL);
- if (strlen(blocker_features) > 0) {
+ if (blocker_features->len > 0) {
+ g_autofree char *blocker_list = NULL;
+
+ g_ptr_array_add(blocker_features, NULL);
+ blocker_list = g_strjoinv(", ", (void *)blocker_features->pdata);
error_setg(&n->migration_blocker,
- "Migration is not supported for %s", blocker_features);
+ "Migration is not supported for %s", blocker_list);
if (migrate_add_blocker(&n->migration_blocker, errp) < 0) {
return false;
}
--
2.47.3On Thu, 9 Jul 2026 at 09:47, Alexander Mikhalitsyn
<alexander@mihalicyn.com> wrote:
>
> From: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
>
> Let's use GPtrArray to build a list of blocker features and then
> g_strjoinv() to build a final comma-delimited string.
>
> While previous approach was technically correct, it is fragile
> (because we need to take care of static buffer size choice) and
> Coverity dislikes it too.
>
> Note, that we use g_ptr_array_new() to allocate array which means
> that GDestroyNotify callback is not set, so we can pass pointers to
> a static memory like g_ptr_array_add(..., (gpointer) "SR-IOV") without
> any problems as there won't be any attempt to free that memory.
>
> Resolves: Coverity CID 1663673
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> ---
> hw/nvme/ctrl.c | 41 +++++++++++++++++------------------------
> 1 file changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index a67e1598891..95a586d82a9 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -9352,22 +9352,11 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
> }
> }
>
> -#define BLOCKER_FEATURES_MAX_LEN 256
> -
> -static inline void nvme_add_blocker_feature(char *blocker_features,
> - const char *feature)
> -{
> - if (strlen(blocker_features) > 0) {
> - g_strlcat(blocker_features, ", ", BLOCKER_FEATURES_MAX_LEN);
> - }
> - g_strlcat(blocker_features, feature, BLOCKER_FEATURES_MAX_LEN);
> -}
> -
> static bool nvme_set_migration_blockers(NvmeCtrl *n, PCIDevice *pci_dev,
> Error **errp)
> {
> uint64_t unsupported_cap, cap = ldq_le_p(&n->bar.cap);
> - char blocker_features[BLOCKER_FEATURES_MAX_LEN] = "";
> + g_autoptr(GPtrArray) blocker_features = g_ptr_array_new();
> bool adm_cmd_security_checked = false;
> bool cmd_io_mgmt_checked = false;
> bool cmd_zone_checked = false;
> @@ -9416,15 +9405,15 @@ static bool nvme_set_migration_blockers(NvmeCtrl *n, PCIDevice *pci_dev,
> }
>
> if (namespaces_num > 1) {
> - nvme_add_blocker_feature(blocker_features,
> - "Namespace Attachment");
> + g_ptr_array_add(blocker_features,
> + (gpointer) "Namespace Attachment");
Is the cast here because we're dropping the const property of the
literal string?
thanks
-- PMM
Am Do., 9. Juli 2026 um 10:51 Uhr schrieb Peter Maydell
<peter.maydell@linaro.org>:
>
> On Thu, 9 Jul 2026 at 09:47, Alexander Mikhalitsyn
> <alexander@mihalicyn.com> wrote:
> >
> > From: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> >
> > Let's use GPtrArray to build a list of blocker features and then
> > g_strjoinv() to build a final comma-delimited string.
> >
> > While previous approach was technically correct, it is fragile
> > (because we need to take care of static buffer size choice) and
> > Coverity dislikes it too.
> >
> > Note, that we use g_ptr_array_new() to allocate array which means
> > that GDestroyNotify callback is not set, so we can pass pointers to
> > a static memory like g_ptr_array_add(..., (gpointer) "SR-IOV") without
> > any problems as there won't be any attempt to free that memory.
> >
> > Resolves: Coverity CID 1663673
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> > ---
> > hw/nvme/ctrl.c | 41 +++++++++++++++++------------------------
> > 1 file changed, 17 insertions(+), 24 deletions(-)
> >
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index a67e1598891..95a586d82a9 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -9352,22 +9352,11 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
> > }
> > }
> >
> > -#define BLOCKER_FEATURES_MAX_LEN 256
> > -
> > -static inline void nvme_add_blocker_feature(char *blocker_features,
> > - const char *feature)
> > -{
> > - if (strlen(blocker_features) > 0) {
> > - g_strlcat(blocker_features, ", ", BLOCKER_FEATURES_MAX_LEN);
> > - }
> > - g_strlcat(blocker_features, feature, BLOCKER_FEATURES_MAX_LEN);
> > -}
> > -
> > static bool nvme_set_migration_blockers(NvmeCtrl *n, PCIDevice *pci_dev,
> > Error **errp)
> > {
> > uint64_t unsupported_cap, cap = ldq_le_p(&n->bar.cap);
> > - char blocker_features[BLOCKER_FEATURES_MAX_LEN] = "";
> > + g_autoptr(GPtrArray) blocker_features = g_ptr_array_new();
> > bool adm_cmd_security_checked = false;
> > bool cmd_io_mgmt_checked = false;
> > bool cmd_zone_checked = false;
> > @@ -9416,15 +9405,15 @@ static bool nvme_set_migration_blockers(NvmeCtrl *n, PCIDevice *pci_dev,
> > }
> >
> > if (namespaces_num > 1) {
> > - nvme_add_blocker_feature(blocker_features,
> > - "Namespace Attachment");
> > + g_ptr_array_add(blocker_features,
> > + (gpointer) "Namespace Attachment");
>
Hi Peter,
> Is the cast here because we're dropping the const property of the
> literal string?
yep.
Compiler output without this cast:
>../hw/nvme/ctrl.c:9418:42: error: passing 'const char[21]' to parameter of type 'gpointer' (aka 'void *') discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
>9418 | "Namespace Attachment");
Kind regards,
Alex
>
> thanks
> -- PMM
On Thu, 9 Jul 2026 at 09:53, Alexander Mikhalitsyn
<alexander@mihalicyn.com> wrote:
>
> Am Do., 9. Juli 2026 um 10:51 Uhr schrieb Peter Maydell
> <peter.maydell@linaro.org>:
> >
> > On Thu, 9 Jul 2026 at 09:47, Alexander Mikhalitsyn
> > <alexander@mihalicyn.com> wrote:
> > >
> > > From: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> > >
> > > Let's use GPtrArray to build a list of blocker features and then
> > > g_strjoinv() to build a final comma-delimited string.
> > >
> > > While previous approach was technically correct, it is fragile
> > > (because we need to take care of static buffer size choice) and
> > > Coverity dislikes it too.
> > >
> > > Note, that we use g_ptr_array_new() to allocate array which means
> > > that GDestroyNotify callback is not set, so we can pass pointers to
> > > a static memory like g_ptr_array_add(..., (gpointer) "SR-IOV") without
> > > any problems as there won't be any attempt to free that memory.
> > >
> > > Resolves: Coverity CID 1663673
> > > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> > > ---
> > > @@ -9416,15 +9405,15 @@ static bool nvme_set_migration_blockers(NvmeCtrl *n, PCIDevice *pci_dev,
> > > }
> > >
> > > if (namespaces_num > 1) {
> > > - nvme_add_blocker_feature(blocker_features,
> > > - "Namespace Attachment");
> > > + g_ptr_array_add(blocker_features,
> > > + (gpointer) "Namespace Attachment");
> >
>
> Hi Peter,
>
> > Is the cast here because we're dropping the const property of the
> > literal string?
>
> yep.
Mmm. Unfortunate but unavoidable I guess.
The glib docs recommend using "void *" instead of "gpointer" in
new code: https://docs.gtk.org/glib/types.html#gpointer
but either way
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
-- PMM
Am Do., 9. Juli 2026 um 10:58 Uhr schrieb Peter Maydell
<peter.maydell@linaro.org>:
>
> On Thu, 9 Jul 2026 at 09:53, Alexander Mikhalitsyn
> <alexander@mihalicyn.com> wrote:
> >
> > Am Do., 9. Juli 2026 um 10:51 Uhr schrieb Peter Maydell
> > <peter.maydell@linaro.org>:
> > >
> > > On Thu, 9 Jul 2026 at 09:47, Alexander Mikhalitsyn
> > > <alexander@mihalicyn.com> wrote:
> > > >
> > > > From: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> > > >
> > > > Let's use GPtrArray to build a list of blocker features and then
> > > > g_strjoinv() to build a final comma-delimited string.
> > > >
> > > > While previous approach was technically correct, it is fragile
> > > > (because we need to take care of static buffer size choice) and
> > > > Coverity dislikes it too.
> > > >
> > > > Note, that we use g_ptr_array_new() to allocate array which means
> > > > that GDestroyNotify callback is not set, so we can pass pointers to
> > > > a static memory like g_ptr_array_add(..., (gpointer) "SR-IOV") without
> > > > any problems as there won't be any attempt to free that memory.
> > > >
> > > > Resolves: Coverity CID 1663673
> > > > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@futurfusion.io>
> > > > ---
>
> > > > @@ -9416,15 +9405,15 @@ static bool nvme_set_migration_blockers(NvmeCtrl *n, PCIDevice *pci_dev,
> > > > }
> > > >
> > > > if (namespaces_num > 1) {
> > > > - nvme_add_blocker_feature(blocker_features,
> > > > - "Namespace Attachment");
> > > > + g_ptr_array_add(blocker_features,
> > > > + (gpointer) "Namespace Attachment");
> > >
> >
> > Hi Peter,
> >
> > > Is the cast here because we're dropping the const property of the
> > > literal string?
> >
> > yep.
>
> Mmm. Unfortunate but unavoidable I guess.
>
> The glib docs recommend using "void *" instead of "gpointer" in
> new code: https://docs.gtk.org/glib/types.html#gpointer
> but either way
ah, I didn't know that. My first intention was to use "void *", but
then I searched over QEMU codebase
and found a few gpointer casts in exact use-case (with g_ptr_array_add).
I can correct this and resend if you want ;)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> -- PMM
© 2016 - 2026 Red Hat, Inc.