> -----Original Message-----
> From: Michal Kubecek [mailto:mkube...@suse.cz]
> Sent: Wednesday, February 6, 2019 4:43 AM
> To: netdev@vger.kernel.org
> Cc: Kirsher, Jeffrey T <jeffrey.t.kirs...@intel.com>; linvi...@tuxdriver.com;
> Nunley, Nicholas D <nicholas.d.nun...@intel.com>; nhor...@redhat.com;
> sassm...@redhat.com
> Subject: Re: [PATCH v2 3/6] ethtool: introduce new ioctl for per-queue
> settings
>
> On Tue, Feb 05, 2019 at 04:01:03PM -0800, Jeff Kirsher wrote:
> > +static int do_perqueue(struct cmd_context *ctx) {
> > + __u32
> queue_mask[__KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32)] = {0};
> > + int i, n_queues = 0;
> > +
> > + if (ctx->argc == 0)
> > + exit_bad_args();
> > +
> > + /*
> > + * The sub commands will be applied to
> > + * all queues if no queue_mask set
> > + */
> > + if (strncmp(*ctx->argp, "queue_mask", 10)) {
>
> This would match any string starting with "queue_mask", is it intended?
No, I'll fix this. I don't know that there are any use cases where this
distinction would matter, but then again there's no reason to have it match
this way.
>
> > + n_queues = find_max_num_queues(ctx);
> > + if (n_queues < 0) {
> > + perror("Cannot get number of queues");
> > + return -EFAULT;
> > + }
> > + for (i = 0; i < n_queues / 32; i++)
> > + queue_mask[i] = ~0;
> > + queue_mask[i] = (1 << (n_queues - i * 32)) - 1;
>
> It's unlikely today, I guess, but in theory, this would overflow if n_queues
> ==
> MAX_NUM_QUEUE
Nice catch, I'll add a check to avoid this.
> > + fprintf(stdout,
> > + "The sub commands will be applied to all %d
> queues\n",
> > + n_queues);
> > + } else {
> > + ctx->argc--;
> > + ctx->argp++;
> > + n_queues = set_queue_mask(queue_mask, *ctx->argp);
> > + if (n_queues < 0) {
> > + perror("Invalid queue mask");
> > + return n_queues;
> > + }
> > + ctx->argc--;
> > + ctx->argp++;
> > + }
> > +
> > + i = find_option(ctx->argc, ctx->argp);
> > + if (i < 0)
> > + exit_bad_args();
> > +
> > + /* no sub_command support yet */
> > +
> > + return 0;
> > +}
>
> Michal Kubecek
