pentapiper/pupdns
1
0
Fork 0
Code Issues Pull requests 1 Projects Releases Packages Wiki Activity Actions

packet-parsing #1

Open
pentapiper wants to merge 5 commits from packet-parsing into main
pull from: packet-parsing
merge into: pentapiper:main
Conversation 6 Commits 5 Files changed 8 +129 -78
pentapiper commented 2025-07-18 16:30:26 -07:00
Owner
Copy link

This change adds basic DNS query header parsing. It should be really fast because it simply casts the byte buffer directly into the header struct, but i had to make some changes to the field order in the struct definition to make this work. Details in src/dns_request.c

This change adds basic DNS query header parsing. It should be really fast because it simply casts the byte buffer directly into the header struct, but i had to make some changes to the field order in the struct definition to make this work. Details in src/dns_request.c
pentapiper added 4 commits 2025-07-18 16:30:26 -07:00
pentapiper added 1 commit 2025-07-18 16:34:00 -07:00
affine reviewed 2025-07-18 18:03:18 -07:00
include/dns_request.h
@ -7,1 +7,4 @@
struct DNSPacketHeader *header;
};
struct DNSRequest *parse_dns_query(struct DNSPacketBuffer *pb);
affine commented 2025-07-18 18:03:18 -07:00
First-time contributor
Copy link

recommend typedef'ing this so that you can use DNSRequest as a first class type name and not need to add "struct" everywhere.

recommend typedef'ing this so that you can use DNSRequest as a first class type name and not need to add "struct" everywhere.
affine reviewed 2025-07-18 18:05:33 -07:00
include/udp_server.h
@ -1,5 +1,5 @@
#pragma once
#define UDP_PORT 53
#define UDP_PORT 1053
affine commented 2025-07-18 18:05:33 -07:00
First-time contributor
Copy link

I think there is a way to leave this at 53 and then use compiler flags to redefine it. Might be a good idea.

Or you could use an IFNDEF clause to have one port for debug builds and one port for release builds based on whether or not cmake is compiling this with debug info.

I think there is a way to leave this at 53 and then use compiler flags to redefine it. Might be a good idea. Or you could use an IFNDEF clause to have one port for debug builds and one port for release builds based on whether or not cmake is compiling this with debug info.
affine reviewed 2025-07-18 18:12:42 -07:00
src/dns_packet_buffer.c
@ -2,3 +2,4 @@
#include <stdio.h>
#include <stdlib.h>
struct DNSPacketBuffer *new_dns_packet_buffer()
affine commented 2025-07-18 18:12:42 -07:00
First-time contributor
Copy link

What is the point of this getter? I could collapse it down to one line:

struct DNSPacketBuffer *buf = (struct DNSPacketBuffer *) malloc(sizeof(struct DNSPacketBuffer));

If you implement a typedef for this that one liner gets even more concise:

DNSPacketBuffer *buf = (DNSPacketBuffer *) malloc(sizeof(DNSPacketBuffer));

Also, I think it is a really bad idea to have malloc called in scattered functions like this. One honest recommendation I can make is to make sure that the point in code where you need memory should be the same place that you both allocate and free it.

In essence: as much as possible every function that calls malloc should also call free.

I have also seen larger applications keep pools associated with lifetimes. NGINX and NGINX Unit both keep a memory pool to allocate out of attached to a request object. Maybe a simpler implementation could be to create a request_resources struct that tracks shared state through the request processing pipeline. You could store pointers to all your malloc()'ed stuff there and then at the very end implement logic that calls free() on everything attached to the request.

What is the point of this getter? I could collapse it down to one line: ```c struct DNSPacketBuffer *buf = (struct DNSPacketBuffer *) malloc(sizeof(struct DNSPacketBuffer)); ``` If you implement a typedef for this that one liner gets even more concise: ```c DNSPacketBuffer *buf = (DNSPacketBuffer *) malloc(sizeof(DNSPacketBuffer)); ``` Also, I think it is a really bad idea to have malloc called in scattered functions like this. One honest recommendation I can make is to make sure that the point in code where you need memory should be the same place that you both allocate and free it. In essence: as much as possible every function that calls malloc should also call free. I have also seen larger applications keep pools associated with lifetimes. NGINX and NGINX Unit both keep a memory pool to allocate out of attached to a request object. Maybe a simpler implementation could be to create a request_resources struct that tracks shared state through the request processing pipeline. You could store pointers to all your malloc()'ed stuff there and then at the very end implement logic that calls free() on everything attached to the request.
affine reviewed 2025-07-18 18:13:14 -07:00
include/dns_packet_buffer.h
@ -5,2 +4,4 @@
#define PB_SIZE 512
#define PB_OUT_OF_BOUNDS 9999;
struct DNSPacketBuffer {
affine commented 2025-07-18 18:13:14 -07:00
First-time contributor
Copy link

Im used to seeing underscores instead of camelcase in C projects, but thats just an opinion.

Im used to seeing underscores instead of camelcase in C projects, but thats just an opinion.
affine reviewed 2025-07-18 18:13:47 -07:00
include/dns_packet_buffer.h
@ -8,4 +9,4 @@
size_t pos;
};
struct DNSPacketBuffer *new_dns_packet_buffer();
affine commented 2025-07-18 18:13:47 -07:00
First-time contributor
Copy link

I think both of the structs in this file should be typedef'ed so that you dont need to write 'struct' everywhere.

I think both of the structs in this file should be typedef'ed so that you dont need to write 'struct' everywhere.
affine requested changes 2025-07-18 18:14:08 -07:00
affine left a comment
First-time contributor
Copy link

Looks cool. I am excited to see how it goes!
Do update your alog dependency to the version on this server though please. I will be deleting the gitlab at some point.

Looks cool. I am excited to see how it goes! Do update your alog dependency to the version on this server though please. I will be deleting the gitlab at some point.
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin packet-parsing:packet-parsing
git checkout packet-parsing

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout main
git merge --no-ff packet-parsing
git checkout packet-parsing
git rebase main
git checkout main
git merge --ff-only packet-parsing
git checkout packet-parsing
git rebase main
git checkout main
git merge --no-ff packet-parsing
git checkout main
git merge --squash packet-parsing
git checkout main
git merge --ff-only packet-parsing
git checkout main
git merge packet-parsing
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
affine
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: pentapiper/pupdns#1
No description provided.
Delete branch "packet-parsing"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?