Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Packet.lua: add clone_to_memory #999

Closed
wants to merge 1 commit into from

Conversation

kbara
Copy link
Contributor

@kbara kbara commented Aug 23, 2016

It's useful to be able to clone packets to arbitrary memory, not just freshly allocated packets. In the lwaftr, we need this facility for cloning packets to within ctables that embed, rather than point to, packets.

@eugeneia

@eugeneia eugeneia self-assigned this Aug 24, 2016
@eugeneia
Copy link
Member

eugeneia commented Aug 24, 2016

How would you feel if I renamed the function to to_pointer (as a counterpart of packet.from_pointer)?

@kbara
Copy link
Contributor Author

kbara commented Aug 24, 2016

I'd be perfectly happy with that; it sounds like a useful improvement.

@eugeneia
Copy link
Member

On second look clone_to_memory does something different than I had thought: it expects the destination to point to a struct packet (it sets dstp.length), so its not really the inverse of from_pointer. Wouldn't it make more sense to add an optional destination arguments to clone and new_packet?

-- Create a new empty packet.
function new_packet (ptr)
   local p = ffi.cast(packet_ptr_t, ptr or memory.dma_alloc(packet_size))
   p.length = 0
   return p
end

-- Create an exact copy of a packet.
function clone (p, p2)
   if not p2 then p2 = allocate() end
   ffi.copy(p2, p, p.length)
   p2.length = p.length
   return p2
end

@kbara
Copy link
Contributor Author

kbara commented Aug 24, 2016

The point, as I originally said, is to have a fully valid struct packet at the end - this originated (albeit in a more complex form) from the needs of the lwaftr's memory-bounded fragment reassembly code.

I don't particularly want to modify new_packet.

I want to be able to clone a packet to a memory address, and have packet.lua take care of all the details that depend on how struct packet is currently defined (in the lwaftr, we use a more complex struct). I don't particularly like adding an extra parameter to clone, because at that point the arguments are src, (optional dst) rather than dst, src as is found in most of the codebase, which is asking for stupid bugs.

@eugeneia
Copy link
Member

eugeneia commented Sep 1, 2016

Closing as per #998.

@eugeneia eugeneia closed this Sep 1, 2016
wingo added a commit that referenced this pull request Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants