MRI solution; is it correct?!

[fattaholmanan]

It seems that the solution provided for the MRI exercise doesn't cover all the details. For example, it's necessary to update totalLen in the IP header anytime the size of packet changes. https://github.com/p4lang/tutorials/blob/119f267938e994bc8053f380231ace79176728c6/P4D2_2017/exercises/mri/solution/mri.p4#L164

Thus, the add_mri_option() action should be something like:

@name("add_mri_option") action add_mri_option() {
   hdr.ipv4_option.setValid();
   hdr.ipv4_option.copyFlag = 1;
   hdr.ipv4_option.optClass = 2;
   hdr.ipv4_option.option = IPV4_OPTION_MRI;
   hdr.ipv4_option.optionLength = 4;

   hdr.mri.setValid();
   hdr.mri.count = 0;
   hdr.ipv4.ihl = hdr.ipv4.ihl + 1;
   hdr.ipv4.totalLen = hdr.ipv4.totalLen + 4;
}

[edgar-costa]

They also have this problem at the Sigcomm2017 tutorial, they are missing:

hdr.ipv4_option.optionLength = hdr.ipv4_option.optionLength + 4;

Otherwise wireshark or scapy are not able to parse properly the header.

[jafingerhut]

Adding label "help wanted" on this one, in hopes that someone with time and interest can investigate whether this is still a problem in latest version of the mri exercise, and fix it, or determine that it has already been fixed.

[Abhinavcode13]

What has to be checked here? @jafingerhut

[jafingerhut]

The first several comments of this issue suggest that there were some lines of code missing in order for the packets to be modified to become correct IPv4 packets. It would be good to find out whether those suggested changes still have not been made, or whether they were made some time after this issue was first created.

[jafingerhut]

I have run the mri exercise solution in solutions/mri.p4 as of 2024-Jul-07, and it appears to be correct, updating the IPv4 header length (field ihl) as required.