Hi Marcel,
On 07/21/2011 02:11 PM, Marcel Holtmann wrote:
Hi Jukka,
+ hdr = (void *) (buf + offset);
+
+ hdr->id = id;
+ hdr->qr = 1;
+ hdr->rcode = 0;
+ hdr->ancount = htons(answers);
+ hdr->nscount = 0;
+ hdr->arcount = 0;
I am still worried about unaligned access here. This might break on
PowerPC for example.
Hmm, actually the same construct is already used in send_response()
function that can be found below the send_cached_response() func. So
there are already problems in committed code if the above code does not
work properly.
that could be well true. I do not run around with a PowerPC machine all
the time ;)
I do not have access to powerpc either :(
+static gboolean cache_check_if_valid(struct cache_data *data,
+ time_t current_time)
+{
+ if (data == NULL)
+ return FALSE;
+
+ if (data->inserted + data->timeout< current_time)
+ return FALSE;
+
+ return TRUE;
+}
+
+static uint16_t cache_check_validity(char *question, uint16_t type,
+ struct cache_data *ipv4,
+ struct cache_data *ipv6)
+{
+ time_t current_time;
+ gboolean valid;
+
+ current_time = time(0);
+
+ if (ipv4 != NULL&& type == 1) {
You already have the ipv4 NULL check in cache_check_if_valid().
I put the NULL check there so that the debug print is not printed and
confuse things.
+ valid = cache_check_if_valid(ipv4, current_time);
+ if (valid == FALSE) {
What is the point of this variable?
if (cache_check_if_valid(ipv4, current_time) == FALSE)
I was using the form without a variable but remembered that the coding
style in connman (at least most places) was to use always a helper
variable. I am confused now :)
+ DBG("cache timeout \"%s\" type A", question);
+
+ /*
+ * We do not remove cache entry if there is still
+ * valid IPv6 entry found in the cache.
+ */
+ valid = cache_check_if_valid(ipv6, current_time);
+ if (valid == FALSE)
+ g_hash_table_remove(cache, question);
+ } else
+ type = 0;
+
+ } else if (ipv6 != NULL&& type == 28) {
+ valid = cache_check_if_valid(ipv6, current_time);
+ if (valid == FALSE) {
+ DBG("cache timeout \"%s\" type AAAA", question);
+
+ valid = cache_check_if_valid(ipv4, current_time);
+ if (valid == FALSE)
+ g_hash_table_remove(cache, question);
+ } else
+ type = 0;
+ }
So what about this:
switch (type) {
case 1: /* IPv4 */
if (cache_check_is_valid(...) == FALSE) {
}
break;
case 28: /* IPv6 */
if (cache_check...
break;
}
Sure. To me the if()'s looked better :)
+static int get_name(int counter,
+ unsigned char *pkt, unsigned char *start, unsigned char *max,
+ unsigned char *output, int output_max, int *output_len,
+ unsigned char **end, char *name, int *name_len)
+{
+ unsigned char *p;
+
+ /* Limit recursion to 10 (this means up to 10 labels in domain name) */
+ if (counter> 10)
+ return -EINVAL;
+
+ p = start;
+ while (*p) {
+ if (*p& NS_CMPRSFLGS) {
+ uint16_t offset = (*p& 0x3F) * 256 + *(p + 1);
+
+ if (offset>= max - pkt)
+ return -ENOBUFS;
+
+ if (*end == NULL)
+ *end = p + 2;
+
+ return get_name(counter + 1, pkt, pkt + offset, max,
+ output, output_max, output_len, end,
+ name, name_len);
+ } else {
+ unsigned label_len = *p;
+
+ if (pkt + label_len> max)
+ return -ENOBUFS;
+
+ if (*output_len> output_max)
+ return -ENOBUFS;
+
+ /*
+ * We need the original name in order to check
+ * if this answer is the correct one.
+ */
+ name[(*name_len)++] = label_len;
+ memcpy(name + *name_len, p + 1, label_len + 1);
+ *name_len += label_len;
+
+ /* We compress the result */
+ output[0] = NS_CMPRSFLGS;
+ output[1] = 0x0C;
+ *output_len = 2;
+
+ p += label_len + 1;
+
+ if (*end == NULL)
+ *end = p;
+
+ if (p>= max)
+ return -ENOBUFS;
+ }
+ }
+
+ return 0;
+}
+
So libresolv has no function for dealing with decoding labels. What
about dn_comp and dn_expand.
dn_expand() resolves the name into presentation format and we do not
want that here. The idea is that we cache the response that is already
in wire format.
Now I am really lost in what you are doing here. This needs more
comments in the code on what is happening.
So the idea was:
- When we get a response from dns server we need to get the ip address
from the packet
- So we parse the message and compare the question part with individual
labels in the dns response. We need to decompress the labels and that is
done with get_name(). We then directly compare the question with the
decompressed label using strcmp(), this can be done because labels are
terminated with \0 even thou they contain chars which ascii value is
<32. The comparison is done in wire format i.e., there is no need to
convert the string to presentation format (replace label lengths to
"."). This comparison is done in parse_response()
And yes, in general saving in wire format is a good idea, but does
dn_expand then destroys the original?
No, but we do not need the presentation format for anything.
+
+ rr = (void *) (*end);
+
+ *type = ntohs(rr->type);
+ *class = ntohs(rr->class);
+ *ttl = ntohl(rr->ttl);
+ *rdlen = ntohs(rr->rdlen);
This is again the question about unaligned access. Are we sure that the
buffer is properly aligned.
Any suggestions how to do this then? The existing code in dnsproxy.c is
made the same way.
It is most likely also wrong then. Or by just luck we have a proper
alignment. Do you happen to have a PowerPC system where you can test
this on? Otherwise it is manual work. Or we have to start using
get_unaligned and set_unaligned.
Perhaps we could go with current solution until we can really test the
alignment issue.
And we check the *class == qclass again here. I am convinced that with a
bit more thinking here this can be done a lot cleaner and easier to read
and understand.
The "*class == qclass" check can be done a bit earlier, that is ok.
I am not sure how much cleaner this code part can be made, I can
certainly add more comments here. This is actually quite tricky parsing
here but that is because of weirdness of the dns packet format.
Just play with it a bit. Right now it is pretty hard to read.
I figure out something.
Regards
Marcel
Jukka
_______________________________________________
connman mailing list
connman@connman.net
http://lists.connman.net/listinfo/connman
