crosser at average
Mar 2, 2009, 1:39 AM
Post #1 of 6
(970 views)
Permalink
|
Gentlemen,
I have a couple of concerns about the new libclamav API introduced in
0.95 (rc1). I understand the reason to remove cl_limits structure, but I
think that the way it was done is, hmm, suboptimal.
cl_engine_set() and cl_engine_get() accessors have void* for the
argument, which may point to different type of variables: uint32, uint64
or char. The type of expected argument is dependent on the value of
cl_engine_field, and there is no type check of any kind, i.e. nothing
that prevents passing of e.g. a char pointer where in32 pointer was
expected. If, by chance, the types of arguments change in a future
release, the user program will recompile cleanly, and the change won't
be noticed. It's actually worse than it was when cl_limits was exposed:
when you assigned a value to a field of cl_limits structure, at least
basic type checking (and/or automatic conversion) was performed.
To mitigate this problem (if you *really* want to get rid of cl_limits
structure exposed to the user), you might introduce separate pairs of
accessor functions for different types of arguments, e.g.:
cl_engine_{get|set}_size(...,uint64_t *val)
cl_engine_{get|set}_int(...,uint32_t *val)
cl_engine_{get|set}_str(...,char *val)
This way, there will be no chance to pass the argument of wrong type.
And here we are coming to my second concern. By requiring the the user
to use bit-size-specific types (uint32_t, uint64_t), you force them to
deploy all the dark magic of having these types defined portably on
different systems, and to essentially duplicate the logic implemented in
cltypes.h. I believe that there is no good reason for that. While there
may be necessary to have bit-size-specific types *inside* clamav, having
them leaking through the API is not justified, in my opinion. I think
that it would be "cleaner" to use more common types in the API, like this:
cl_engine_{get|set}_size(...,off_t *val)
cl_engine_{get|set}_int(...,int *val)
cl_engine_{get|set}_str(...,char *val)
And a final note: I think it's worth mentioning in the documentation
what is the relation between "options" passed to cl_init() and "options"
passed to scanning functions. If they are different, maybe it's better
to name them differently, like "init_options" and "scan_options".
Thanks for consideration,
Regards,
Eugene
|
signature.asc
