[prev in list] [next in list] [prev in thread] [next in thread]
List: strongswan-announce
Subject: Re: [strongSwan-dev] [Patch] support dynamic IP adresses
From: Mirko Parthey <mirko.parthey () informatik ! tu-chemnitz ! de>
Date: 2011-08-31 14:50:23
Message-ID: 20110831145023.GA17768 () guitar ! localdomain
[Download RAW message or body]
Hello Martin,
thanks for reviewing and improving my patch.
On Mon, Aug 29, 2011 at 10:12:51AM +0200, Martin Willi wrote:
> The patch looks fine, might be a little more complicated than required,
> though:
>
> > + char *dnsname;
> > + bool has_dnsname;
>
> Using "host" as member is probably easier, as it refers to the KW_HOST
> keyword. The has_dnsname attribute could be eliminated by just setting
> dnsname/host to NULL(?).
OK.
> > - plog("# bad addr: %s=%s [%s]", name, value, ugh);
> > - if (streq(ugh, "does not look numeric and name lookup failed"))
> > - {
> > - end->dns_failed = TRUE;
>
> I think it would be a good idea to accept name resolution failures and
> just pass on the DNS name to charon. This would allow us to use the
> connection even if the name is not resolvable during startup.
I agree, but could not come up with an elegant way to limit this to charon.
Your checking for IKEv2 looks like the right thing to do.
> I've attached a patch containing these modifications. I'll push it if it
> looks fine to you.
Please see my comments below.
> - { ARG_MISC, 0, NULL /* KW_HOST */ },
> + { ARG_STR, offsetof(starter_end_t, host), NULL },
You dropped the /* KW_HOST */ comment - intention or accident?
> + else
> + {
> + /* check for allow_any prefix */
> + if (value[0] == '%')
> + {
> + end->allow_any = TRUE;
> + value++;
> + }
> + conn->addr_family = ip_version(value);
> + ugh = ttoaddr(value, 0, conn->addr_family, &end->addr);
> + if (ugh != NULL)
> + {
> + plog("# bad addr: %s=%s [%s]", name, value, ugh);
> + if (streq(ugh, "does not look numeric and name lookup failed"))
> + {
> + end->dns_failed = TRUE;
> + anyaddr(conn->addr_family, &end->addr);
> + }
> + else
> + {
> + goto err;
> + }
> + }
> + if (!end->allow_any)
> + {
> + end->host = clone_str(value);
> + }
DNS names can be given with or without a '%' prefix, and I would expect
> + end->host = clone_str(value);
to be executed in either case. The (!end->allow_any) conditional looks
superfluous to me.
> @@ -464,7 +470,7 @@ static void handle_dns_failure(const char *label, starter_end_t *end,
> plog("# fallback to %s=%%any due to '%%' prefix or %sallowany=yes",
> label, label);
> }
> - else
> + else if (!end->host || conn->keyexchange != KEY_EXCHANGE_IKEV2)
> {
> /* declare an error */
> cfg->err++;
What is your intention behind the !end->host conditional here?
Is it related to the !end->allow_any discussed above?
Regards,
Mirko
_______________________________________________
Dev mailing list
Dev@lists.strongswan.org
https://lists.strongswan.org/mailman/listinfo/dev
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic