[Openvas-devel] [Openvas-commits] r17541 - in trunk/openvas-libraries: . base

Henri Doreau henri.doreau at gmail.com
Thu Sep 5 22:25:26 CEST 2013


Hi Hani,

these changes look promising! Here are some comments after a quick review.

2013/9/3  <scm-commit at wald.intevation.org>:
> Author: kroosec
> Date: 2013-09-03 11:23:48 +0200 (Tue, 03 Sep 2013)
> New Revision: 17541
>
> [...]
> +/* Static values */
> +
> +enum {
> +  HOST_TYPE_NAME = 0,       /* Hostname eg. foo */
> +  HOST_TYPE_IPV4,           /* eg. 192.168.1.1 */
> +  HOST_TYPE_CIDR_BLOCK,     /* eg. 192.168.15.0/24 */
> +  HOST_TYPE_RANGE_SHORT,    /* eg. 192.168.15.10-20 */
> +  HOST_TYPE_RANGE_LONG,     /* eg. 192.168.15.10-192.168.18.3 */
> +  HOST_TYPE_IPV6,           /* eg. ::1 */
> +  HOST_TYPE_MAX             /* Boundary checking. */
> +};
> +
> +/* Typedefs */
> +typedef struct openvas_host openvas_host_t;
> +typedef struct openvas_hosts openvas_hosts_t;
> +
> +/* Data structures. */
> +
> +/**
> + * @brief The structure for a single host object.
> + *
> + * The elements of this structure should never be accessed directly.
> + * Only the functions corresponding to this module should be used.
> + */
> +struct openvas_host
> +{
> +  union {
> +    gchar *name;            /* Hostname. */
> +    struct in_addr addr;    /* IPv4 address */
> +    struct in6_addr addr6;  /* IPv6 address */
> +  };
> +  unsigned int type;  /* HOST_TYPE_NAME, HOST_TYPE_IPV4 or HOST_TYPE_IPV6. */
> +};
What about giving HOST_TYPE_* enum a name (enum host_type for
instance) and replacing this last unsigned int by a enum host_type?

I'd also suggest using a variable-length structure, so that you don't
have to allocate memory twice when storing a hostname.

I tend to find the name of the structure openvas_hosts (plural) is
error prone, but why not, it challenges code reviewers! ;) Also, why
isn't the last field unsigned ('removed')? I might be missing
something but the fact that you have two GList* pointers indicates
that an intrusive data structure would be better. Not sure whether
glib provides one though.

In openvas_hosts.c, you should be careful when returning right after
inet_pton() statements. This function can set errno, you might want to
either protect errno or document the behavior.

In openvas_hosts_remove_duplicates(), don't declare variables anywhere
but at the beginning of a block (cf. GList *element). I dislike the
dedup algorithm but you already highlighted its drawbacks in your
comment. If I understand correctly, you're expanding everything, which
is very expensive in term of memory. I'd suggest using iterators to
solve both the CPU and memory issue. I think you can split the target
specifications into a set of sorted iterators, one per contiguous
range.

I have other suggestions but I'd need more time to determine which are
relevant or not. Could you first tell us more about this HG
replacement?

Regards

-- 
Henri



More information about the Openvas-devel mailing list