Re: [PATCH for 7.0 V10 6/6] net/net.c: Add handler for passthrough filter command

[Markus Armbruster]

This is not review; I'm merely pointing out errors that caught my eye.

Zhang Chen <chen.zhang@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.zhang@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;
> +}

[...]