Hi Hani,
these changes look promising! Here are some comments after a quick review.
2013/9/3 <scm-com...@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
_______________________________________________
Openvas-devel mailing list
Openvas-devel@wald.intevation.org
https://lists.wald.intevation.org/cgi-bin/mailman/listinfo/openvas-devel
