I'm creating some functions that work with a struct that simulates an ethernet header.

typedef struct ethernet_hdr_ {
    dir_mac_t dst_mac;
    dir_mac_t src_mac;
    short type;
    char payload[MAX_SIZE_PAYLOAD];
    unsigned int FCS;
} ethernet_hdr_t;

#define ETH_HDR_SIZE_EXCL_PAYLOAD (sizeof(ethernet_hdr_t) - sizeof(((ethernet_hdr_t *)0)->payload))

I need to define the next function.

static inline ethernet_hdr_t * ALLOC_ETH_HDR_WITH_PAYLOAD(char *pkt, unsigned int pkt_size)

According to this assignment, which I already did, but I want to know whether there is something wrong with my solution.

The above API must encapsulate the existing DATA into the payload of ethernet header, i.e. above API must return a pointer to the ethernet hdr the payload of which carries the data of size pkt_size pointed by pkt in above diagram.

The layout of data with new ethernet hdr must look like given in the image Q3 attached in the instruction of this assignment. Initialize all the fields of the new ethernet hdr (including FCS) exclusing payload to zero.

This is Q3.

This is how I did it.

static inline ethernet_hdr_t * ALLOC_ETH_HDR_WITH_PAYLOAD(char *pkt, unsigned int pkt_size) {
    ethernet_hdr_t *ethernet_hdr = calloc(1, sizeof(ethernet_hdr_t));
    memcpy(ethernet_hdr->payload, pkt, pkt_size);       
    return ethernet_hdr;
}

However, I found the official solution to this assignment, and it is very different from mine.

static inline ethernet_hdr_t * ALLOC_ETH_HDR_WITH_PAYLOAD(char *pkt, unsigned int pkt_size) {    
    char *temp = calloc(1, pkt_size);
    memcpy(temp, pkt, pkt_size);    
    ethernet_hdr_t *eth_hdr = (ethernet_hdr_t *)(pkt - ETH_HDR_SIZE_EXCL_PAYLOAD);
    memset((char *)eth_hdr, 0, ETH_HDR_SIZE_EXCL_PAYLOAD);
    memcpy(eth_hdr->payload, temp, pkt_size);    
    free(temp);
    return eth_hdr;
}

Clearly, my function is much simpler but I think it is missing something. So, I'm wondering whether both solutions are correct, but even if they are, maybe the second one is better.

CodePudding user response：

In your case you are creating a new object to store the packet header and payload (frame) data, setting the header to zeros and copying the payload from pkt to the new frame data object payload member.

In the "official" solution, the program assumes that pkt points to the payload member of an object of type ethernet_hdr_t. Then, in a very complicated fashion, it attempts to do the same thing as your code, but using the original object of which pkt is a member.

Overall, I would say that the "official" solution is the problematic for several reasons:

1. It assumes that the compiler will arrange the contents of struct ethernet_hdr_ in a specific way, but this is actually defined by the implementation. This is due to the macro ETH_HDR_SIZE_EXCL_PAYLOAD using the object and member payload sizes to attempt to locate payload within the struct. A safer way might be to use the standard library offsetof macro to determine the offset of member payload within the struct.
2. Assuming that pkt is a member of another object type is not a good programming practice. For example, assume that the payload contents were copied to another object, which is not a member of ethernet_hdr_t prior to calling ALLOC_ETH_HDR_WITH_PAYLOAD. This would result in a memory access violation. The function should only attempt to access the objects provided as parameters and not objects that these objects "may" be members of.
3. The use of temp in this function does not make much sense. The payload data is effectively copied to temp then copied back to pkt indirectly through the object pointed to by eth_hdr.
4. The object generating the inputs to ALLOC_ETH_HDR_WITH_PAYLOAD is being updated by the function and the returned pointer will point to the same object. This could lead to some confusion by anyone trying to use the function.

In the case of your code, it accomplishes the same thing as the "official" answer, but creates a new object to contain the data. You will need to be sure that the memory is properly deallocated once the object is no longer needed, but when comparing the two, I would argue that your code is more correct in that it will not result in undefined, implementation-defined or confusing behavior.