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

Optional encapsulation format in Ethernet II and SNAP #341

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rogeroger-yu
Copy link

The encapsulation of LLDP protocal can be switched
between Ethernet II and SNAP.

Issue: 340
Change-Id: Ia010ddb72e959be9312f0de6eb2fe4a0586a8d0a

@rogeroger-yu rogeroger-yu force-pushed the feature/add_snap_format branch 2 times, most recently from 86c9f60 to 7431cc8 Compare September 3, 2019 10:28
The encapsulation of LLDP protocal can be switched
between Ethernet and snap.

Issue: 340
Change-Id: Ia010ddb72e959be9312f0de6eb2fe4a0586a8d0a
Copy link
Member

@vincentbernat vincentbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks OK for me. It is missing tests, but I can add them later.

} else if (set->local_lldp_snap == 0) {
log_debug("rpc", "Disable SNAP encapsulation format");
port->p_lldp_snap = 0;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think any port-related command will disable SNAP encapsulation. You should use 1 (true), 2 (false) instead.

@@ -686,6 +686,7 @@ typedef enum {
lldpctl_k_port_id_subtype, /**< `(IS)` The subtype ID of this port. */
lldpctl_k_port_id, /**< `(BS,WO)` The ID of this port. */
lldpctl_k_port_descr, /**< `(S,WO)` The description of this port. */
lldpctl_k_port_lldp_snap_enabled, /**< `(I,WO)` The lldp snap encapsulation enable status of this port. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not put a new value in the middle of existing values. It breaks the ABI (old clients cannot talk to new lldpd). Add it at the end of the section.

memcpy(packet + 2*ETHER_ADDR_LEN, &type, bytes))
#define POKE_REAL_LENGTH_REPLACE(value, bytes) \
POKE_REAL_LENGTH(value, types.f_uint16, bytes, htons)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These macros are used only in one place. I think this is better to put the code directly instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants