On 9/19/05, Juan RP <juan@xtrarom.org> wrote:
> On Mon, 19 Sep 2005 13:36:03 +0200
> "Julio M. Merino Vidal" <jmmv84@gmail.com> wrote:
> > +     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.
>=20
> I cannot use the link set here, because I need to check the flags used
> in the driver's bufq structure.

Yes you can.  You seem to have changed the patch in the FTP, so I
cannot access the original code any more to comment about this specific
question (I can't remember its original contents).

Anyway, almost all your constructions can be reduced to a simple loop.
The following one in pseudocode illustrates how you'd use different flags,
assuming these flags are stored in the link set (and if they are not, chang=
e
the link set to hold them):

boolean found =3D false
for each i in link set
    if i->name matches the name set by the user
        use the value of i->flag as the flag you need
        found =3D true
        break loop
    end if
end for
if not found, error

> > +     if (strcmp(dks->dks_schedname, "fcfs") =3D=3D 0)
> > +             bqflag =3D BUFQ_FCFS;
> >
> > Same as above (avoid "big switches"), and also use strncmp.
> > We'd get false positives otherwise (based on incorrect user
> > input).
>=20
> What do you suggest to use rather than "big switches"?

Use the link sets as shown above.

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

Imagine the following situation: in the future, we want to extend our
system by adding a BUFQ_FOO strategy.  Adding this should be as
easy as creating a new bufq_foo.c file, compiling it and linking it
against the kernel.

After your changes, adding this BUFQ_FOO becomes more difficult
and intrusive, because you have to change other code in the system
that is not related at all to the new foo strategy.

Keep this in mind: your patch must not have any of "fcfs", BUFQ_FCFS
nor other names in it to be correct.  It has to use values provided by
the link sets and by the user, exclusively.

I'd suggest you to read some book about basic software design
principles, which often focus on extensibility and maintenanceability
(hmm, does this last word exist?).

> > 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.
>=20
> Why is that suboptimal?

Because the user does not have a way to easily parse the results from
the kernel.  What if he wants to see if a strategy is available, instead of
printing the results?

Conceptually, the kernel is returning a list of items, which the user can
then manage at will.  You are imposing a restriction, assuming that all
the user wants is a screen-printable string with all strategies' names in
it.  In other words, a string is not "designed" to hold lists; there are
better data structures to do that, such as, e.g., vectors.

--=20
Julio M. Merino Vidal <jmmv84@gmail.com>
http://www.livejournal.com/users/jmmv/
The NetBSD Project - http://www.NetBSD.org/