In the interest of time, here are my first high-level commments on
libdlpi.c. I'd like these to be resolved before doing a more thorough
review. I'd hope that applying these comments could reduce libdlpi.c
by at least 300 lines.
* Most of libdlpi.c consists of functions with this form:
1. Figure out size of request and reply buffers.
2. Allocate buffers.
3. Do some function-specific setup.
4. Call dlpi_msg_common().
5. Do some function-specific and generic processing of the results
of dlpi_msg_common().
6. Free buffers.
Much more care needs to be put into minimizing redundant code, and
streamlining code that is not redundant. In most cases, simply
allocating buffers on the stack and defining helper routines would
greatly reduce code sprawl. For instance, dlpi_unbind() is currently
57 lines of fairly boilerplate code -- but with some helper functions
and more careful allocation, could be reduced to 21 lines:
int
dlpi_unbind(dlpi_handle_t dh)
{
int retval;
dl_unbind_req_t *dlubrp;
union DL_primitives *replyp;
dlpi_impl_t *dip = (dlpi_impl_t *)dh;
if (dip == NULL)
return (DLPI_EINHANDLE);
replyp = alloca(DLPI_ACK_REPLY_SIZE);
dlubrp = alloca(DL_UNBIND_REQ_SIZE);
dlubrp->dl_primitive = DL_UNBIND_REQ;
retval = dlpi_msg_common(dip, dlubrp, DL_UNBIND_REQ_SIZE, replyp,
DL_OK_ACK, DLPI_ACK_REPLY_SIZE, DL_OK_ACK_SIZE, NULL);
if (retval != DLPI_SUCCESS)
return (retval);
return (dlpi_retval(replyp));
}
... where dlpi_retval() is defined as:
int
dlpi_retval(union DL_primitives *replyp)
{
dl_error_ack_t *dleap = (dl_error_ack_t *)replyp;
switch (replyp->dl_primitive) {
case DL_OK_ACK:
return (DLPI_SUCCESS);
case DL_ERROR_ACK:
errno = dleap->dl_unix_errno;
return (dleap->dl_errno);
default:
errno = EBADMSG;
return (DL_SYSERR);
}
}
... and DLPI_ACK_REPLY_SIZE is a #define for MAX(sizeof (dl_ok_ack_t),
sizeof (dl_error_ack_t).
This same pattern can be repeated for i_dlpi_passive(),
i_dlpi_multi(), i_dlpi_promisc(), dlpi_info(), dlpi_bind(),
dlpi_get_physaddr(), dlpi_set_physaddr(), dlpi_attach(), and
dlpi_detach(). You should be able to haul out over many hundreds of
lines of code.
* There appear to be needless extra type indirections. For instance,
the code has:
typedef enum dlpi_multi_op {
DLPI_MULTI_DISABLE = 0,
DLPI_MULTI_ENABLE
} dlpi_multi_op_t;
... but there's no advantage to this over just directly passing the
primitive to i_dlpi_multi(). In fact, i_dlpi_multi() can be made
simpler because you can simply assign `reqprimp->dl_primitive = op'.
* There's a fair amount of strange code associated with determining the
request and reply buffer size. For instance, i_dlpi_promisc()
includes this:
opsize = (op == DLPI_PROMISC_ON) ? sizeof (dl_promiscon_req_t) :
sizeof (dl_promiscoff_req_t);
size = 0;
size = MAX(opsize, size);
size = MAX(sizeof (dl_ok_ack_t), size);
size = MAX(sizeof (dl_error_ack_t), size);
if ((reqprimp = malloc(size)) == NULL)
return (DLPI_ENOMEM);
if ((repprimp = malloc(size)) == NULL) {
free(reqprimp);
return (DLPI_ENOMEM);
}
... but as per the DLPI spec, only dl_ok_ack_t or dl_error_ack_t will
be returned in a reply -- so I cannot figure out why the reply is
allocated to potentially holda dl_promiscon_req_t. Further, as per my
previous comments, alloca() can be used to allocate small stack-local
buffers of dynamic size (you could also use some C99 features to do
this, but I'm not sure if those are generall allowed in ON at this
point). So, I'd expect the above code to simply be:
if (op == DL_PROMISCON_REQ)
reqprimp = alloca(dl_promiscon_req_t);
else
reqprimp = alloca(dl_promiscoff_req_t);
repprimp = alloca(DLPI_ACK_REPLY_SIZE);
* I'm not sure why dli_passive and dli_raw are stored in separate
fields. The code would be simpler if you had a dli_oflags field
that was assigned to `flags' at open time.
--
meem
