Comments and feedback on UDP tutorial
petervwyatt opened this issue · 10 comments
You say "Each is of two bytes, but in our specification, we treat the source port, destination port, and length fields as integers." but I'm unclear if this an arbitrary decision or a conscious decision for a special reason (and on a further assumption that "int" is effectively of arbitrary size and signed and not merely an int32). I would assume that recommended practice is to always stay as true to the spec as possible, define a new 16-bit unsigned type (which will involve endianness concerns) and then use that?
You also define checksum differently as [byte] whereas it is also a 16-bit value according to the spec - so why not an int like the previous 3 fields?
Again, is this an arbitrary decision or a conscious decision for a special reason I haven't read to yet?
Does Parsley support named constants? You use "8" when subtracting off the header but magic numbers are never good practice...
Is this also where if you'd defined the source, dest, length and checksum as UInt16 you could have used a sizeof() operation equivalent?
Is the "Byte" type signed or unsigned or does it not matter?
At the end of the 1st UDP tutorial you don't make any statements about range checking or validating the checksum.
Is it true that because of the UInt16<endian=endian::Big()> and all port values (0 to 2^16-1) being valid that no range checking is necessary on source and destination?
Why don't you validate the checksum? If this is in a later lesson just say so :-)
The file in ./test/lang/udp.py is not precisely the same as shown in the tutorial.
Specifically the [length >= 8] line is below the data = (Byte^[length-8)) line - does that make any difference? (since the length underflow check is after the data assignment)???
I got to the end of the tutorial but didn't get a chance to run parsleyc.exe.
I also discovered the -p option and notice that this produces something close but not exactly the same as udp.ply
Just realised I was using a different udp.ply - there are 2 that are slightly different:
diff doc/tutorial/examples/udp.ply test/lang/udp.ply
Thank you for all the feedback; Prashanth and I spoke about the comments and I'm providing our thoughts below.
QUESTION:
You say "Each is of two bytes, but in our specification, we treat the source port, destination port, and length fields as integers." but I'm unclear if this an arbitrary decision or a conscious decision for a special reason (and on a further assumption that "int" is effectively of arbitrary size and signed and not merely an int32). I would assume that recommended practice is to always stay as true to the spec as possible, define a new 16-bit unsigned type (which will involve endianness concerns) and then use that?
RESPONSE:
We used ints in the user-defined type, which is the data-structure the
user manipulates. In the grammar portion, we used UInt16(), so the
parsing would be correct as per spec.
The main reason we only have ints in Parsley is to avoid having to
define a numeric tower (int8, uint8, int16, uint16, ...) and the
conversions between them all. That's a lot of tedious engineering,
without much core research value. We could add that engineering once
the more interesting grammar issues are worked out.
QUESTION:
You also define checksum differently as [byte] whereas it is also a 16-bit value according to the spec - so why not an int like the previous 3 fields?
Again, is this an arbitrary decision or a conscious decision for a special reason I haven't read to yet?
RESPONSE: Now that we have added bit support, I think we can capture the checksum as a bit string.
The earlier rationale to define checksum as a [byte] was as follows: The checksum doesn't make sense as an integer, since it is only compared for equality, and no arithmetic is performed
on it once computed.
QUESTION:
Does Parsley support named constants? You use "8" when subtracting off the header but magic numbers are never good practice...
RESPONSE:
Agreed. We don't yet have named constants, and it would be good to add.
QUESTION:
Is this also where if you'd defined the source, dest, length and checksum as UInt16 you could have used a sizeof() operation equivalent?
RESPONSE:
Good point. We'll shift to using appropriately named temporaries instead of magic numbers.
QUESTION:
Is the "Byte" type signed or unsigned or does it not matter?
RESPONSE:
It is unsigned. We'll update Parsley to convey this.
QUESTION:
At the end of the 1st UDP tutorial you don't make any statements about range checking or validating the checksum.
Is it true that because of the UInt16<endian=endian::Big()> and all port values (0 to 2^16-1) being valid that no range checking is necessary on source and destination?
Why don't you validate the checksum? If this is in a later lesson just say so :-)
RESPONSE:
Yes! We do plan to do this. We were postponing this bit until we added bit support to Parsley, but we now should be able to do checksum validation. Thanks for the reminder.
QUESTION:
The file in ./test/lang/udp.py is not precisely the same as shown in the tutorial.
Specifically the [length >= 8] line is below the data = (Byte^[length-8)) line - does that make any difference? (since the length underflow check is after the data assignment)???
RESPONSE:
Good catch! We will fix this. Yes, this is indeed important for graceful failure, as you note.
QUESTION:
I got to the end of the tutorial but didn't get a chance to run parsleyc.exe.
I also discovered the -p option and notice that this produces something close but not exactly the same as udp.ply
RESPONSE:
-p is mainly a debugging option for Parsley developers and power
users, and not really meant for most users.
QUESTION:
Just realised I was using a different udp.ply - there are 2 that are slightly different:
diff doc/tutorial/examples/udp.ply test/lang/udp.ply
RESPONSE:
Ah, good catch---thank you for notifying us!
Thanks for your reply. I'm happy with all your responses so feel free to close this issue whenever you wish...
Sounds good. Thanks again.