On Mon, 19 Sep 2005 13:36:03 +0200
"Julio M. Merino Vidal" <jmmv84@gmail.com> wrote:

> > Is this ok? is there something wrong?
> 
> +		if (argv[0] != NULL)
> +			strcpy(dks.dks_schedname, argv[0]);
> 
> This can overflow the target string, causing a segfault in the
> program. Use strlcpy setting a limit of sizeof(dks.dks_schedname).
> There are several other strcpy's in the code with the same problem.
> 
> +        	if (ioctl(fd, DIOCSIOSCHED, &dks) == -1)
> +                	err(1, "%s: scheduler", dvname);
> 
> Use EXIT_FAILURE as the error code.  (Same problem in other calls
> to err.)

Fixed.

> +	switch (strat) {
> +	case BUFQ_FCFS:
> +		strcpy(dks->dks_schedname, "fcfs");
> +		break;
> +	case BUFQ_DISKSORT:
> +		strcpy(dks->dks_schedname, "disksort");
> +		break;
> [...]
> 
> This is not scalable.  The kernel already has a 'bufq_strats' link
> sets that holds all available link sets, changeable at build time.
> Iterate over that link set (__link_set_foreach) to do what you need.
> (See sys/bufq.h.)  Note: I don't know if that link set alone will be
> enough for your patch, but if it is not, it should be changed or
> a new one added.  Having big switches in the code with hardcoded
> values in them goes against extensibility.

I cannot use the link set here, because I need to check the flags used
in the driver's bufq structure.

> +	if (strcmp(dks->dks_schedname, "fcfs") == 0)
> +		bqflag = BUFQ_FCFS;
> 
> Same as above (avoid "big switches"), and also use strncmp.
> We'd get false positives otherwise (based on incorrect user
> input).

What do you suggest to use rather than "big switches"? 

I'm just using the BUFQ_ definitions, I don't understand why
you mean "hardcoded values".

> Now, I see that in disk_list_sched you already iterate over the
> link set.  However, the switch inside it is wired.  Note that the
> items in the link set already carry out the name of the item, which
> you can reuse.
> 
> +	char		dks_schedname[32];	/* disk
> scheduler name */
> +	char		dks_methodname[32];	/* sorting
> method name */
> 
> I'd define a constant holding the value 32 instead of using the value
> directly.

Fixed.

> And another thing: in the operation to list schedulers, DIOCLIOSCHED,
> returning a single string with the whole list in it is suboptimal.
> Instead, change it to return a vector (if it was dynamically sized,
> that'd be superb, but I'm not sure how you'd do it) so that the user
> can iterate over it and print its items as he wishes.

Why is that suboptimal?

Thanks.