comments in-line below
On 2015-02-11 15:59, David Mandelberg wrote:
> From: David Mandelberg <dmand...@bbn.com>
>
> Use the schema introduced in the previous set of commits when
> querying ROAs from the database.
>
> WARNING: This is one of a series of commits to use the updated schema.
> It is not the last in the series, so the code is still broken.
>
> addresses [#6] and [#7]
> ---
> bin/rpki/query.c | 4 +-
> lib/rpki/err.c | 1 +
> lib/rpki/err.h | 3 +-
> lib/rpki/querySupport.c | 228
> ++++++++++++++++++++++++++++++++++++++++++++++--
> lib/rpki/querySupport.h | 4 +-
> 5 files changed, 229 insertions(+), 11 deletions(-)
>
> diff --git a/bin/rpki/query.c b/bin/rpki/query.c
> index 70e600d..4af5b60 100644
> --- a/bin/rpki/query.c
> +++ b/bin/rpki/query.c
> @@ -91,7 +91,6 @@ static int handleResults(
> char resultStr[MAX_RESULT_SZ];
> int i;
>
> - UNREFERENCED_PARAMETER(conp);
This should be in a separate commit dedicated to changing the
displayfunc typedef.
> UNREFERENCED_PARAMETER(numLine);
> if (validate)
> {
> @@ -108,7 +107,8 @@ static int handleResults(
> QueryField *field = globalFields[display];
> if (field->displayer != NULL)
> {
> - result += field->displayer(s, result, resultStr);
> + result += field->displayer(
> + scmp, conp, s, result, resultStr);
This should be in a separate commit dedicated to changing the
displayfunc typedef.
> }
> else if (s->vec[result].avalsize != SQL_NULL_DATA)
> {
> diff --git a/lib/rpki/err.c b/lib/rpki/err.c
> index 9b078c1..f184546 100644
> --- a/lib/rpki/err.c
> +++ b/lib/rpki/err.c
> @@ -180,6 +180,7 @@ static char *errs[-(ERR_SCM_MAXERR) + 1] = {
> "AIA in TA cert", /* ERR_SCM_AIATA */
> "Invalid signed attribute", /* ERR_SCM_INVALSATTR */
> "TA cert has inherit resources", /* ERR_SCM_TAINHERIT */
> + "Truncated data", /* ERR_SCM_TRUNCATED */
This should be in a separate commit dedicated to adding a new error
code.
> };
>
> char *err2string(
> diff --git a/lib/rpki/err.h b/lib/rpki/err.h
> index 7313904..835b624 100644
> --- a/lib/rpki/err.h
> +++ b/lib/rpki/err.h
> @@ -169,7 +169,8 @@
> #define ERR_SCM_AIATA -160 /* AIA found in TA cert */
> #define ERR_SCM_INVALSATTR -161 /* Invalid signed attribute */
> #define ERR_SCM_TAINHERIT -162 /* TA cert has inherit resources */
> -#define ERR_SCM_MAXERR -162
> +#define ERR_SCM_TRUNCATED -163 /* Truncated data */
> +#define ERR_SCM_MAXERR -163
This sould be in a separate commit dedicated to adding a new error
code.
For a later time: Using defines like this is awkward. An approach I
prefer is to let an enum handle the number assignment, which also has
the advantage of providing debug symbols to make it easier to
troubleshoot problems in gdb. For example:
// err.h
/**
* @brief helper macro to avoid typing out the list of error codes
* multiple times
*/
#define ERROR_CODES(f) \
f(ERR_SCM_NOERR, "No error") // this entry must be first \
f(ERR_SCM_COFILE, "Cannot open file") \
f(ERR_SCM_NOMEM, "Out of memory") \
... \
f(ERR_SCM_TRUNCATED, "Truncated data") \
// end of error codes list
#define ERROR_ENUM_POS(NAME, DESCR) POS_##NAME,
#define ERROR_ENUM(NAME, DESCR) NAME = -POS_##NAME,
#define ERROR_ARRAY_DESCR(NAME, DESCR) [POS_##NAME] = DESCR,
#define ERROR_ARRAY_NAME(NAME, DESCR) [POS_##NAME] = #NAME,
/**
* @brief helper enum to make it easier to create the error_code enum
*
* do not use this -- use the error_code enum entries instead
*/
enum {
ERROR_CODES(ERROR_ENUM_POS)
};
/**
* @brief error codes (and one success code)
*
* Only zero is a success code; all other values indicate an error.
*/
typedef enum {
ERROR_CODES(ERROR_ENUM)
} error_code;
// err.c
/**
* @brief array mapping negated error codes to human-friendly
* descriptions
*/
static const char *errs[] = {
ERROR_CODES(ERROR_ARRAY_DESCR)
};
/**
* @brief array mapping negated error codes to their names
*/
static const char *err_names[] = {
ERROR_CODES(ERROR_ARRAY_NAME)
};
Then, whenever an error code is used, you can use the error_code
type instead of int (or ssize_t or something else):
error_code
foo(...)
{
...
if ((bar = malloc(xyz)) == NULL)
{
return ERR_SCM_NOMEM;
}
...
return ERR_SCM_NOERR;
}
>
> /*
> * macro that prints an error string and call return if a condition is true
> diff --git a/lib/rpki/querySupport.c b/lib/rpki/querySupport.c
> index 1e6c6c3..bc4f4da 100644
> --- a/lib/rpki/querySupport.c
> +++ b/lib/rpki/querySupport.c
> @@ -42,6 +42,7 @@ void addQueryFlagTests(
> * all these static variables are used for efficiency, so that
> * there is no need to initialize them with each call to checkValidity
> */
> +static scmtab *roaPrefixTable = NULL;
A similar line (with similar initialization code) was added in
lib/rpki/sqhl.c. It would be nice to not have the code duplicated.
> static scmtab *validTable = NULL;
> static scmsrcha *validSrch = NULL,
> *anySrch = NULL;
> @@ -203,10 +204,14 @@ int checkValidity(
> * combines dirname and filename into a pathname
> */
> static int pathnameDisplay(
> + scm * scmp,
> + scmcon * connection,
This should be in a separate commit dedicated to changing the
displayfunc typedef.
> scmsrcha * s,
> int idx1,
> char *returnStr)
> {
> + (void)scmp;
> + (void)connection;
This should be in a separate commit dedicated to changing the
displayfunc typedef.
> snprintf(returnStr, MAX_RESULT_SZ, "%s/%s",
> (char *)s->vec[idx1].valptr, (char *)s->vec[idx1 + 1].valptr);
> return 2;
> @@ -216,10 +221,15 @@ static int pathnameDisplay(
> * create space-separated string of serial numbers
> */
> static int displaySNList(
> + scm * scmp,
> + scmcon * connection,
I think this should be in a separate commit dedicated to changing the
displayfunc typedef.
> scmsrcha * s,
> int idx1,
> char *returnStr)
> {
> + (void)scmp;
> + (void)connection;
> +
I think this should be in a separate commit dedicated to changing the
displayfunc typedef.
> uint8_t *snlist;
> unsigned int i,
> snlen;
> @@ -249,6 +259,198 @@ static int displaySNList(
> return 2;
> }
>
> +struct display_ip_addrs_context
> +{
> + char const * separator;
> +
> + char * result;
> + size_t result_len;
> + size_t result_idx;
> +};
Needs Doxygen documentation. (At least something like, "callback state
for the display_ip_addrs_valuefunc() callback function".)
> +
> +static int display_ip_addrs_valuefunc(
> + scmcon * conp,
> + scmsrcha * s,
> + ssize_t idx)
Needs Doxygen documentation. (At least something like, "searchscm()
callback for display_ip_addrs(), called for each prefix in a ROA".)
> +{
> + (void)conp;
> + (void)idx;
> +
> + struct display_ip_addrs_context * context = s->context;
> +
> + uint8_t const * prefix = s->vec[0].valptr;
Should be unsigned char instead of uint8_t according to the
documentation for SQL_C_BINARY.
> + int_fast64_t const prefix_family_length = s->vec[0].avalsize;
Why int_fast64_t? Why not size_t? Or SQLLEN to match the type of the
struct member in scmsrch?
> + unsigned const prefix_length =
> + *(unsigned const *)s->vec[1].valptr;
> + unsigned const prefix_max_length =
> + *(unsigned const *)s->vec[2].valptr;
These should be 'unsigned long' according to the documentation for
SQL_C_ULONG
Also, you don't need the const in the cast.
> +
> + int af;
> + switch (prefix_family_length)
> + {
> + case 4:
> + af = AF_INET;
> + break;
> +
> + case 16:
> + af = AF_INET6;
> + break;
What if another address family has length 4 or 16? Instead of making
these assumptions, there should be an address family column in the
database.
> +
> + default:
> + LOG(LOG_ERR, "invalid prefix_family_length %" PRIuFAST64,
> + prefix_family_length);
> + return ERR_SCM_INTERNAL;
> + }
> +
> + char prefix_str[INET6_ADDRSTRLEN];
> + if (inet_ntop(af, prefix, prefix_str, sizeof(prefix_str)) == NULL)
This file should #include <arpa/inet.h> to get inet_ntop() and
INET6_ADDRSTRLEN.
> + {
> + LOG(LOG_ERR, "error converting prefix to a string");
> + return ERR_SCM_INTERNAL;
> + }
> +
> + int snprintf_ret = snprintf(
> + context->result + context->result_idx,
> + context->result_len - context->result_idx,
> + "%s/%u(%u)%s",
> + prefix_str,
> + prefix_length,
> + prefix_max_length,
> + context->separator);
> + if (snprintf_ret < 0)
> + {
> + return ERR_SCM_INTERNAL;
> + }
> + else if ((size_t)snprintf_ret >=
> + context->result_len - context->result_idx)
> + {
> + return ERR_SCM_TRUNCATED;
This gets awkward if the context->separator on the last prefix is what
tips the snprintf() over context->result_len. Then you might end up
with something like:
foo, bar, baz...
or
foo, bar, baz,...
when baz is the last entry. Instead it would be better to:
1. initialize context->separator to the empty string in
display_ip_addrs(),
2. have display_ip_addrs_valuefunc() do:
int snprintf_ret = snprintf(
context->result + context->result_idx,
context->result_len - context->result_idx,
"%s%s/%u(%u)",
context->separator,
prefix_str,
prefix_length,
prefix_max_length);
context->separator = ", ";
This way you never have to undo the final separator in
display_ip_addrs(), and you only get the ERR_SCM_TRUNCATED error if
valuable data is actually truncated.
> + }
> + else
> + {
> + context->result_idx += snprintf_ret;
> + }
> +
> + return 0;
> +}
> +
> +static int display_ip_addrs(
> + scm * scmp,
> + scmcon * connection,
> + scmsrcha * s,
> + int idx1,
> + char *returnStr)
Needs Doxygen documentation.
Specifically, is the caller required to guarantee that returnStr[0] is
0? If not, what happens if there are no prefixes in a ROA? The
caller will get a potentially unterminated garbage string back.
> +{
> + static char const truncated_str[] = "...";
> + static char const separator[] = ", ";
> + static size_t const separator_len = sizeof(separator) - 1;
> +
> + if (roaPrefixTable == NULL)
> + {
> + roaPrefixTable = findtablescm(scmp, "ROA_PREFIX");
What if roaPrefixTable is still NULL here?
> + }
> +
> + int sta;
> +
> + unsigned int roa_local_id =
> + *((unsigned int *)(s->vec[idx1].valptr));
How do you know that valptr holds a pointer to an unsigned int, given
that the sqlType field in the QueryField associated with this callback
is -1?
Assuming that the sqlType field will be changed to SQL_C_ULONG, this
should be an unsigned long, not an unsigned int.
> + char roa_local_id_str[24];
What's the significance of 24 here? (please avoid magic numbers, or
at least add a comment explaining the choice)
> + snprintf(roa_local_id_str, sizeof(roa_local_id_str), "%u",
> + roa_local_id);
When a particular call to snprintf() is expected to return a
non-negative number less than its second arg, then it would be nice to
use a wrapper around snprintf() that assert()s this rather than
disregard the return value.
> +
> + struct display_ip_addrs_context context;
> + context.separator = separator;
> + context.result = returnStr;
> + context.result_len = MAX_RESULT_SZ - sizeof(truncated_str);
Why reserve space for the truncation indicator? Why not just
overwrite the last few value bytes if it becomes truncated?
> + context.result_idx = 0;
I think it's cleaner to use an initializer:
struct display_ip_addrs_context context = {
.separator = separator,
.result = returnStr,
.result_len = MAX_RESULT_SZ - sizeof(truncated_str),
.result_idx = 0,
};
> +
> + uint8_t prefix[16];
According to the ODBC docs, SQL_C_BINARY gets written to an unsigned
char array, not a uint8_t array. (There is no difference if uint8_t
is supported on the target platform, but it should still match the
documentation.)
Please avoid magic numbers.
> + unsigned prefix_length;
> + unsigned prefix_max_length;
Shouldn't these be unsigned long? (The search below uses SQL_C_ULONG
with these.)
> +
> + scmsrch select[3];
Please avoid magic numbers and do something like this instead:
scmsrch select[] = {
{
.colno = 1,
.sqltype = SQL_C_BINARY,
.colname = "prefix",
.valptr = prefix,
.valsize = sizeof(prefix),
.avalsize = 0,
},
...
};
> + scmkv where_cols[1];
Similarly:
scmkv where_cols[] = {
{.column = "roa_local_id", .value = roa_local_id_str},
};
> + scmkva where;
I think it's cleaner to use an initializer here.
> + char * order;
> + scmsrcha srch;
Same here.
> +
> + select[0].colno = 1;
> + select[0].sqltype = SQL_C_BINARY;
> + select[0].colname = "prefix";
> + select[0].valptr = prefix;
> + select[0].valsize = sizeof(prefix);
> + select[0].avalsize = 0;
> + select[1].colno = 2;
> + select[1].sqltype = SQL_C_ULONG;
> + select[1].colname = "prefix_length";
> + select[1].valptr = &prefix_length;
> + select[1].valsize = sizeof(prefix_length);
> + select[1].avalsize = 0;
> + select[2].colno = 3;
> + select[2].sqltype = SQL_C_ULONG;
> + select[2].colname = "prefix_max_length";
> + select[2].valptr = &prefix_max_length;
> + select[2].valsize = sizeof(prefix_max_length);
> + select[2].avalsize = 0;
> +
> + where_cols[0].column = "roa_local_id";
> + where_cols[0].value = roa_local_id_str;
> +
> + where.vec = where_cols;
> + where.ntot = sizeof(where_cols)/sizeof(where_cols[0]);
> + where.nused = where.ntot;
> + where.vald = 0;
> +
> + order =
> + "length(prefix) asc, "
> + "prefix asc, "
> + "prefix_length asc, "
> + "prefix_max_length asc";
> +
> + srch.vec = select;
> + srch.sname = NULL;
> + srch.ntot = sizeof(select)/sizeof(select[0]);
> + srch.nused = srch.ntot;
> + srch.vald = 0;
> + srch.where = &where;
> + srch.wherestr = NULL;
> + srch.context = &context;
> +
> + sta = searchscm(
> + connection,
> + roaPrefixTable,
> + &srch,
> + NULL,
> + display_ip_addrs_valuefunc,
> + SCM_SRCH_DOVALUE_ANN | SCM_SRCH_BREAK_VERR,
> + order);
> + if (sta == ERR_SCM_TRUNCATED)
Note for the future: The documentation for searchscm() should be
updated to say that if the SCM_SRCH_BREAK_VERR flag is set and the
callback returns non-zero, then searchscm() will return the value
returned by the callback. (I'm assuming this is true based on the
above usage.)
> + {
> + snprintf(
> + context.result + context.result_idx,
> + MAX_RESULT_SZ - context.result_idx,
> + "%s",
> + truncated_str);
I don't like the ignored return value.
> + }
> + else if (sta < 0)
What if (sta > 0)? Rather than check for error, it's safer to check
for non-success, for example: else if (sta != 0)
> + {
> + // XXX: there should be a better way to signal an error
Agreed. The displayfunc typedef should be changed to return an error
code, and have an out parameter that does whatever the return value
currently does (which is unclear to me).
> + snprintf(
> + context.result,
> + MAX_RESULT_SZ,
> + "error: %s (%d)",
> + err2string(sta),
> + sta);
I don't like the ignored return value.
> + }
> + else if (context.result_idx >= separator_len)
> + {
> + // Remove the final separator
> + context.result[context.result_idx - separator_len] = '\0';
> + }
What terminates context.result (returnStr) if
display_ip_addrs_valuefunc() is never called because there are no
prefixes associated with the ROA?
> +
> + return 1;
What does returning 1 mean?
> +}
> +
> /*
> * helper function for displayFlags
> */
> @@ -292,10 +494,14 @@ void setIsManifest(
> * create list of all flags set to true
> */
> static int displayFlags(
> + scm * scmp,
> + scmcon * connection,
This should be in a separate commit dedicated to changing the
displayfunc typedef.
> scmsrcha * s,
> int idx1,
> char *returnStr)
> {
> + (void)scmp;
> + (void)connection;
This should be in a separate commit dedicated to changing the
displayfunc typedef.
> unsigned int flags = *((unsigned int *)(s->vec[idx1].valptr));
> returnStr[0] = 0;
> addFlagIfSet(returnStr, flags, SCM_FLAG_CA, "CA");
> @@ -391,14 +597,22 @@ static QueryField fields[] = {
> "CRLDP", NULL,
> },
> {
> - "ip_addrs", /* name of the field */
> + "local_id",
> + NULL,
> + Q_JUST_DISPLAY | Q_FOR_ROA,
> + SQL_C_ULONG, 4,
Shouldn't maxSize be sizeof(unsigned long)?
> + NULL, NULL,
> + NULL, NULL,
> + },
> + {
> + "ip_addrs",
> "the set of IP addresses assigned by the ROA",
> - Q_JUST_DISPLAY | Q_FOR_ROA, /* flags */
> - SQL_C_CHAR, 32768, /* sql return type, size */
> - NULL, /* use this for query, not name */
> - NULL, /* second field for query */
> - "IP Addresses", /* name of column for printout */
> - NULL, /* function for display string */
> + Q_JUST_DISPLAY | Q_FOR_ROA,
> + -1, 0,
Why is sqlType -1? What does that mean? The unixODBC documentation
(and Microsoft's ODBC documentation) only specifies SQL_C_*, not -1:
http://www.unixodbc.org/doc/ProgrammerManual/Tutorial/gloss.html#dtyp
Shouldn't sqlType be SQL_C_ULONG since this is actually querying the
local_id column?
And shouldn't maxSize be sizeof(unsigned long)?
> + "local_id",
> + NULL,
> + "IP Addresses",
> + display_ip_addrs,
> },
> {
> "asn",
> diff --git a/lib/rpki/querySupport.h b/lib/rpki/querySupport.h
> index 2a27e2e..91a98cb 100644
> --- a/lib/rpki/querySupport.h
> +++ b/lib/rpki/querySupport.h
> @@ -18,6 +18,8 @@ extern void addQueryFlagTests(
> /****** prototype for a function for displaying a field *****/
> typedef int (
> *displayfunc) (
> + scm * scmp,
> + scmcon * connection,
A few things:
* This should be in a separate commit dedicated to changing the
displayfunc typedef.
* Please add Doxygen documentation for these two new arguments.
(Don't worry about documenting the others, but if you do, please
do it in another commit.)
* For a later time: This typedef really needs Doxygen
documentation. (especially return value and idx1 parameter)
* For a later time: This typedef should be a function type, not a
pointer to a function type. For example:
/**
* @brief Doxygen documentation goes here
*/
typedef int displayfunc(
scm *scmp,
scmcon *connection,
scmsrcha *s,
int idx1,
char *returnStr);
Leaving out the asterisk makes it possible to declare and then
define functions like so:
/**
* @brief Doxygen documentation goes here
*/
static displayfunc display_ip_addrs;
int
display_ip_addrs(
scm *scmp,
scmcon *connection,
scmsrcha *s,
int idx1,
char *returnStr)
{
...
}
This serves a few purposes:
1. If the typedef ever becomes out of sync with the actual
function signature, you'll get an obvious compile error.
2. The purpose of the function becomes obvious. Because it has
type 'displayfunc' it implicitly follows the specificiation
for the displayfunc typedef (parameter semantics, return
value, etc.).
3. It makes it easy to grep the code to find all of the
functions that are used as callbacks for a particular
purpose.
> scmsrcha * s,
> int idx1,
> char *returnStr);
> @@ -78,4 +80,4 @@ void setIsManifest(
> #define Q_FOR_MAN 0x20
> #define Q_FOR_GBR 0x40
>
> -#define MAX_RESULT_SZ 8192
> +#define MAX_RESULT_SZ (128 * 1024)
This should be in a separate commit.
>
------------------------------------------------------------------------------
_______________________________________________
rpstir-devel mailing list
rpstir-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/rpstir-devel
