This is not review; I'm merely pointing out errors that caught my eye.
Zhang Chen <chen.zh...@intel.com> writes:
> Use the connection protocol,src port,dst port,src ip,dst ip as the key
> to passthrough certain network traffic in object with network packet
> processing function.
>
> Signed-off-by: Zhang Chen <chen.zh...@intel.com>
> ---
> net/net.c | 199 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 197 insertions(+), 2 deletions(-)
>
> diff --git a/net/net.c b/net/net.c
> index 5d0d5914fb..443e88d396 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -55,6 +55,8 @@
> #include "net/colo-compare.h"
> #include "net/filter.h"
> #include "qapi/string-output-visitor.h"
> +#include "net/colo-compare.h"
> +#include "qom/object_interfaces.h"
>
> /* Net bridge is currently not supported for W32. */
> #if !defined(_WIN32)
> @@ -1215,14 +1217,207 @@ void qmp_netdev_del(const char *id, Error **errp)
> }
> }
>
> +static int check_addr(InetSocketAddressBase *addr)
> +{
> + if (!addr || (addr->host && !qemu_isdigit(addr->host[0]))) {
> + return -1;
> + }
> +
> + if (atoi(addr->port) > 65536 || atoi(addr->port) < 0) {
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +/* The initial version only supports colo-compare */
> +static CompareState *passthrough_filter_check(IPFlowSpec *spec, Error **errp)
> +{
> + Object *container;
> + Object *obj;
> + CompareState *s;
> +
> + if (!spec->object_name) {
How can spec->object_name ever be null? It's not optional in the QAPI
schema.
> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "object-name",
> + "Need input object name");
> + return NULL;
> + }
> +
> + container = object_get_objects_root();
> + obj = object_resolve_path_component(container, spec->object_name);
As far as I can tell, object_resolve_path_component()'s second argument
is a property name, *not* a path. I think you want
s = (CompareState *)object_resolve_path_type(spec->object_name,
COLO_COMPARE, NULL);
This also takes care of ...
> + if (!obj) {
> + error_setg(errp, "object '%s' not found", spec->object_name);
> + return NULL;
> + }
> +
> + s = COLO_COMPARE(obj);
... a probable bug here: when the object exists (@obj is not null), but
isn't a TYPE_COLO_COMPARE object, then @s is null here. We can then
return null without setting an error.
> +
> + if (!getprotobyname(spec->protocol)) {
> + error_setg(errp, "Passthrough filter get wrong protocol");
> + return NULL;
> + }
> +
> + if (spec->source) {
> + if (check_addr(spec->source)) {
> + error_setg(errp, "Passthrough filter get wrong source");
> + return NULL;
> + }
> + }
> +
> + if (spec->destination) {
> + if (check_addr(spec->destination)) {
> + error_setg(errp, "Passthrough filter get wrong destination");
> + return NULL;
> + }
> + }
> +
> + return s;
> +}
> +
> +/* The initial version only supports colo-compare */
Is there another version in the tree? I guess not. Recommend
/* Supports only colo-compare so far */
Such limitations need to be clearly stated in the QAPI schema doc
comments.
> +static COLOPassthroughEntry *passthrough_filter_find(CompareState *s,
> + COLOPassthroughEntry
> *ent)
> +{
> + COLOPassthroughEntry *next = NULL, *origin = NULL;
> +
> + if (!QLIST_EMPTY(&s->passthroughlist)) {
> + QLIST_FOREACH_SAFE(origin, &s->passthroughlist, node, next) {
> + if ((ent->l4_protocol.p_proto == origin->l4_protocol.p_proto) &&
> + (ent->src_port == origin->src_port) &&
> + (ent->dst_port == origin->dst_port) &&
> + (ent->src_ip.s_addr == origin->src_ip.s_addr) &&
> + (ent->dst_ip.s_addr == origin->dst_ip.s_addr)) {
> + return origin;
> + }
> + }
> + }
> +
> + return NULL;
> +}
[...]
