In for-loop in net_init_af_xdp, we do nc->queue_index = i,
where is is int64_t for 0 to queues-1, and nc->queue_index is
unsigned int.
Also in parse_socket_fds, g_strv_length() returns guint which
is equivalent to unsigned int.
Let's simply use unsigned int type for queues, and update
the check appropriately.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
net/af-xdp.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/net/af-xdp.c b/net/af-xdp.c
index 14f302ea21..bb7a7d8e33 100644
--- a/net/af-xdp.c
+++ b/net/af-xdp.c
@@ -442,14 +442,14 @@ static NetClientInfo net_af_xdp_info = {
};
static int *parse_socket_fds(const char *sock_fds_str,
- int64_t n_expected, Error **errp)
+ unsigned n_expected, Error **errp)
{
gchar **substrings = g_strsplit(sock_fds_str, ":", -1);
- int64_t i, n_sock_fds = g_strv_length(substrings);
+ unsigned i, n_sock_fds = g_strv_length(substrings);
int *sock_fds = NULL;
if (n_sock_fds != n_expected) {
- error_setg(errp, "expected %"PRIi64" socket fds, got %"PRIi64,
+ error_setg(errp, "expected %u socket fds, got %u",
n_expected, n_sock_fds);
goto exit;
}
@@ -484,7 +484,7 @@ int net_init_af_xdp(const Netdev *netdev,
unsigned int ifindex;
uint32_t prog_id = 0;
g_autofree int *sock_fds = NULL;
- int64_t i, queues;
+ unsigned i, queues;
Error *err = NULL;
AFXDPState *s;
bool inhibit;
@@ -496,13 +496,14 @@ int net_init_af_xdp(const Netdev *netdev,
return -1;
}
- queues = opts->has_queues ? opts->queues : 1;
- if (queues < 1) {
+ if (opts->has_queues && (opts->queues < 0 || opts->queues > UINT_MAX)) {
error_setg(errp, "invalid number of queues (%" PRIi64 ") for '%s'",
- queues, opts->ifname);
+ opts->queues, opts->ifname);
return -1;
}
+ queues = opts->has_queues ? opts->queues : 1;
+
inhibit = opts->has_inhibit && opts->inhibit;
if (inhibit && !opts->sock_fds && !opts->map_path) {
error_setg(errp, "'inhibit=on' requires 'sock-fds' or 'map-path'");
@@ -537,7 +538,7 @@ int net_init_af_xdp(const Netdev *netdev,
for (i = 0; i < queues; i++) {
nc = qemu_new_net_client(&net_af_xdp_info, peer, "af-xdp", name);
- qemu_set_info_str(nc, "af-xdp%"PRIi64" to %s", i, opts->ifname);
+ qemu_set_info_str(nc, "af-xdp%u to %s", i, opts->ifname);
nc->queue_index = i;
if (!nc0) {
--
2.52.0
> In for-loop in net_init_af_xdp, we do nc->queue_index = i,
> where is is int64_t for 0 to queues-1, and nc->queue_index is
> unsigned int.
> Also in parse_socket_fds, g_strv_length() returns guint which
> is equivalent to unsigned int.
> Let's simply use unsigned int type for queues, and update
> the check appropriately.
I have some concerns about this patch queues is generally signed,
so negative values can be used for error handling. We should
probably be consistent with this.
> - queues = opts->has_queues ? opts->queues : 1;
> - if (queues < 1) {
> + if (opts->has_queues && (opts->queues < 0 || opts->queues > UINT_MAX)) {
> error_setg(errp, "invalid number of queues (%" PRIi64 ") for '%s'",
> - queues, opts->ifname);
> + opts->queues, opts->ifname);
> return -1;
> }
Perhaps the condition here should be opts->queues > MAX_TAP_QUEUES
instead of UINT_MAX
Thanks,
Ben
On 03.02.26 22:10, Chaney, Ben wrote:
>
>> In for-loop in net_init_af_xdp, we do nc->queue_index = i,
>> where is is int64_t for 0 to queues-1, and nc->queue_index is
>> unsigned int.
>
>
>> Also in parse_socket_fds, g_strv_length() returns guint which
>> is equivalent to unsigned int.
>
>
>> Let's simply use unsigned int type for queues, and update
>> the check appropriately.
>
>
> I have some concerns about this patch queues is generally signed,
> so negative values can be used for error handling.
Right, and because of it, in "net: introduce net_parse_fds()" I
change this UINT_MAX limit to INT_MAX.
> We should
> probably be consistent with this.
I wanted make it int, but decided, that I don't want to argue against
If you're using "int" or "long", odds are good that there's a better type.
If a variable is counting something, it should be declared with an
unsigned type.
of docs/devel/style.rst :)
Still, in tap.c, queues variable is int in net_init_tap(), exactly to handle
errors. Still, where possible I use unsigned for it. Don't know what is better.
No problem to change all "queues" to be int, up to Jason.
>
>
>> - queues = opts->has_queues ? opts->queues : 1;
>> - if (queues < 1) {
>> + if (opts->has_queues && (opts->queues < 0 || opts->queues > UINT_MAX)) {
>> error_setg(errp, "invalid number of queues (%" PRIi64 ") for '%s'",
>> - queues, opts->ifname);
>> + opts->queues, opts->ifname);
>> return -1;
>> }
>
> Perhaps the condition here should be opts->queues > MAX_TAP_QUEUES
> instead of UINT_MAX
>
> Thanks,
> Ben
>
--
Best regards,
Vladimir
On 2/3/26 8:10 PM, Chaney, Ben wrote:
>
>> In for-loop in net_init_af_xdp, we do nc->queue_index = i,
>> where is is int64_t for 0 to queues-1, and nc->queue_index is
>> unsigned int.
>
>
>> Also in parse_socket_fds, g_strv_length() returns guint which
>> is equivalent to unsigned int.
>
>
>> Let's simply use unsigned int type for queues, and update
>> the check appropriately.
>
>
> I have some concerns about this patch queues is generally signed,
> so negative values can be used for error handling. We should
> probably be consistent with this.
>
>
>> - queues = opts->has_queues ? opts->queues : 1;
>> - if (queues < 1) {
>> + if (opts->has_queues && (opts->queues < 0 || opts->queues > UINT_MAX)) {
>> error_setg(errp, "invalid number of queues (%" PRIi64 ") for '%s'",
>> - queues, opts->ifname);
>> + opts->queues, opts->ifname);
>> return -1;
>> }
>
> Perhaps the condition here should be opts->queues > MAX_TAP_QUEUES
> instead of UINT_MAX
I wonder why would we use MAX_TAP_QUEUES for something that isn't
a tap interface? It's also not defined here, would be a bit awkward
to export.
Best regards, Ilya Maximets.
> >
> > Perhaps the condition here should be opts->queues > MAX_TAP_QUEUES
> > instead of UINT_MAX
> I wonder why would we use MAX_TAP_QUEUES for something that isn't
> a tap interface? It's also not defined here, would be a bit awkward
> to export.
Fair point. It looks like I got my wires crossed a bit here. (And also in my
comment to the other patch). Would it be possible to have different
MAX_XYZ_QUEUES for other device types?
Thanks,
Ben
On 2/3/26 9:21 PM, Chaney, Ben wrote: >>> >>> Perhaps the condition here should be opts->queues > MAX_TAP_QUEUES >>> instead of UINT_MAX > > >> I wonder why would we use MAX_TAP_QUEUES for something that isn't >> a tap interface? It's also not defined here, would be a bit awkward >> to export. > > Fair point. It looks like I got my wires crossed a bit here. (And also in my > comment to the other patch). Would it be possible to have different > MAX_XYZ_QUEUES for other device types? Why do we need an artificial limit on the number of queues? In the kernel the number of queues per device is not limited, it just needs to fit into unsigned int. So, I don't think we need to create artificial limits on QEMU side. AFAIU, the MAX_TAP_QUEUES was introduced because implementation allocated an array of that size on stack and it is not wise to do so with a fairly unbound values. But today even tap.c allocates everything dynamically, so I'm not even sure if the MAX_TAP_QUEUES makes much sense for net/tap itself, as it seems to be an artificial limit for the number of fds that can be passed in. It is also no enforced when QEMU creates the queues on its own. Best regards, Ilya Maximets.
On 04.02.26 01:53, Ilya Maximets wrote: > On 2/3/26 9:21 PM, Chaney, Ben wrote: >>>> >>>> Perhaps the condition here should be opts->queues > MAX_TAP_QUEUES >>>> instead of UINT_MAX >> >> >>> I wonder why would we use MAX_TAP_QUEUES for something that isn't >>> a tap interface? It's also not defined here, would be a bit awkward >>> to export. >> >> Fair point. It looks like I got my wires crossed a bit here. (And also in my >> comment to the other patch). Would it be possible to have different >> MAX_XYZ_QUEUES for other device types? > > Why do we need an artificial limit on the number of queues? > In the kernel the number of queues per device is not limited, > it just needs to fit into unsigned int. So, I don't think > we need to create artificial limits on QEMU side. > > AFAIU, the MAX_TAP_QUEUES was introduced because implementation > allocated an array of that size on stack and it is not wise > to do so with a fairly unbound values. But today even tap.c > allocates everything dynamically, so I'm not even sure if the > MAX_TAP_QUEUES makes much sense for net/tap itself, as it seems > to be an artificial limit for the number of fds that can be > passed in. It is also no enforced when QEMU creates the queues > on its own. > > Best regards, Ilya Maximets. Right, MAX_TAP_QUEUES is artificial for tap.c, and becomes absolutely unused after this series. Still, I forget to remove the macro itself. -- Best regards, Vladimir
On 1/29/26 10:32 PM, Vladimir Sementsov-Ogievskiy wrote:
> In for-loop in net_init_af_xdp, we do nc->queue_index = i,
> where is is int64_t for 0 to queues-1, and nc->queue_index is
> unsigned int.
>
> Also in parse_socket_fds, g_strv_length() returns guint which
> is equivalent to unsigned int.
>
> Let's simply use unsigned int type for queues, and update
> the check appropriately.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> net/af-xdp.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/net/af-xdp.c b/net/af-xdp.c
> index 14f302ea21..bb7a7d8e33 100644
> --- a/net/af-xdp.c
> +++ b/net/af-xdp.c
> @@ -442,14 +442,14 @@ static NetClientInfo net_af_xdp_info = {
> };
>
> static int *parse_socket_fds(const char *sock_fds_str,
> - int64_t n_expected, Error **errp)
> + unsigned n_expected, Error **errp)
> {
> gchar **substrings = g_strsplit(sock_fds_str, ":", -1);
> - int64_t i, n_sock_fds = g_strv_length(substrings);
> + unsigned i, n_sock_fds = g_strv_length(substrings);
> int *sock_fds = NULL;
>
> if (n_sock_fds != n_expected) {
> - error_setg(errp, "expected %"PRIi64" socket fds, got %"PRIi64,
> + error_setg(errp, "expected %u socket fds, got %u",
> n_expected, n_sock_fds);
> goto exit;
> }
> @@ -484,7 +484,7 @@ int net_init_af_xdp(const Netdev *netdev,
> unsigned int ifindex;
> uint32_t prog_id = 0;
> g_autofree int *sock_fds = NULL;
> - int64_t i, queues;
> + unsigned i, queues;
> Error *err = NULL;
> AFXDPState *s;
> bool inhibit;
> @@ -496,13 +496,14 @@ int net_init_af_xdp(const Netdev *netdev,
> return -1;
> }
>
> - queues = opts->has_queues ? opts->queues : 1;
> - if (queues < 1) {
> + if (opts->has_queues && (opts->queues < 0 || opts->queues > UINT_MAX)) {
Not a full review, but this changes the logic. Zero is not a valid number
of queues.
> error_setg(errp, "invalid number of queues (%" PRIi64 ") for '%s'",
> - queues, opts->ifname);
> + opts->queues, opts->ifname);
> return -1;
> }
>
> + queues = opts->has_queues ? opts->queues : 1;
> +
> inhibit = opts->has_inhibit && opts->inhibit;
> if (inhibit && !opts->sock_fds && !opts->map_path) {
> error_setg(errp, "'inhibit=on' requires 'sock-fds' or 'map-path'");
> @@ -537,7 +538,7 @@ int net_init_af_xdp(const Netdev *netdev,
>
> for (i = 0; i < queues; i++) {
> nc = qemu_new_net_client(&net_af_xdp_info, peer, "af-xdp", name);
> - qemu_set_info_str(nc, "af-xdp%"PRIi64" to %s", i, opts->ifname);
> + qemu_set_info_str(nc, "af-xdp%u to %s", i, opts->ifname);
> nc->queue_index = i;
>
> if (!nc0) {
On 30.01.26 02:15, Ilya Maximets wrote:
> On 1/29/26 10:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>> In for-loop in net_init_af_xdp, we do nc->queue_index = i,
>> where is is int64_t for 0 to queues-1, and nc->queue_index is
>> unsigned int.
>>
>> Also in parse_socket_fds, g_strv_length() returns guint which
>> is equivalent to unsigned int.
>>
>> Let's simply use unsigned int type for queues, and update
>> the check appropriately.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> net/af-xdp.c | 17 +++++++++--------
>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/af-xdp.c b/net/af-xdp.c
>> index 14f302ea21..bb7a7d8e33 100644
>> --- a/net/af-xdp.c
>> +++ b/net/af-xdp.c
>> @@ -442,14 +442,14 @@ static NetClientInfo net_af_xdp_info = {
>> };
>>
>> static int *parse_socket_fds(const char *sock_fds_str,
>> - int64_t n_expected, Error **errp)
>> + unsigned n_expected, Error **errp)
>> {
>> gchar **substrings = g_strsplit(sock_fds_str, ":", -1);
>> - int64_t i, n_sock_fds = g_strv_length(substrings);
>> + unsigned i, n_sock_fds = g_strv_length(substrings);
>> int *sock_fds = NULL;
>>
>> if (n_sock_fds != n_expected) {
>> - error_setg(errp, "expected %"PRIi64" socket fds, got %"PRIi64,
>> + error_setg(errp, "expected %u socket fds, got %u",
>> n_expected, n_sock_fds);
>> goto exit;
>> }
>> @@ -484,7 +484,7 @@ int net_init_af_xdp(const Netdev *netdev,
>> unsigned int ifindex;
>> uint32_t prog_id = 0;
>> g_autofree int *sock_fds = NULL;
>> - int64_t i, queues;
>> + unsigned i, queues;
>> Error *err = NULL;
>> AFXDPState *s;
>> bool inhibit;
>> @@ -496,13 +496,14 @@ int net_init_af_xdp(const Netdev *netdev,
>> return -1;
>> }
>>
>> - queues = opts->has_queues ? opts->queues : 1;
>> - if (queues < 1) {
>> + if (opts->has_queues && (opts->queues < 0 || opts->queues > UINT_MAX)) {
>
> Not a full review, but this changes the logic. Zero is not a valid number
> of queues.
Oops, good shot, thanks for looking. Will fix.
>
>> error_setg(errp, "invalid number of queues (%" PRIi64 ") for '%s'",
>> - queues, opts->ifname);
>> + opts->queues, opts->ifname);
>> return -1;
>> }
>>
>> + queues = opts->has_queues ? opts->queues : 1;
>> +
>> inhibit = opts->has_inhibit && opts->inhibit;
>> if (inhibit && !opts->sock_fds && !opts->map_path) {
>> error_setg(errp, "'inhibit=on' requires 'sock-fds' or 'map-path'");
>> @@ -537,7 +538,7 @@ int net_init_af_xdp(const Netdev *netdev,
>>
>> for (i = 0; i < queues; i++) {
>> nc = qemu_new_net_client(&net_af_xdp_info, peer, "af-xdp", name);
>> - qemu_set_info_str(nc, "af-xdp%"PRIi64" to %s", i, opts->ifname);
>> + qemu_set_info_str(nc, "af-xdp%u to %s", i, opts->ifname);
>> nc->queue_index = i;
>>
>> if (!nc0) {
>
--
Best regards,
Vladimir
© 2016 - 2026 Red Hat, Inc.