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

add ct-zone filtering inat src host #16

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mansish404
Copy link

No description provided.

@mansish404 mansish404 force-pushed the ovn/add-src-host-changes branch from 4133437 to a7976ff Compare September 16, 2024 06:26

ht = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, NULL);
for (i = 0; i < num_ct_zones; i++) {
LOG(VERBOSE, "%s : Trying to insert %u ct_zone into ht", __func__,
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can remove this log.

or define:

int ct_zone = atoi(ct_zones_list[i])
and reuse in L101 and L103

@@ -19,6 +19,11 @@

#include "log.h"


enum filter_type {
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing documentation.

enum filter_type {
FILTER_BY_CT_ZONE,
FILTER_BY_IP
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: add line after this line.

@@ -95,7 +95,6 @@ conntrack_store_insert(struct conntrack_store *conn_store,
{
uint32_t ct_id;
struct conntrack_entry *ct_entry;

Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated.

@@ -116,8 +115,6 @@ conntrack_store_insert(struct conntrack_store *conn_store,
}

/**
* Deletes the entry from the conntrack_store.
Copy link
Collaborator

Choose a reason for hiding this comment

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

???

__func__, strerror(ret));
}

dump_conntrack(targs);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason to use event args here? those were meant for different threads.

you can simply pass ct_zones_to_migrate as second arg right?

for IP case you can set it to NULL/empty:

dump_conntrack(ips_to_migrate, NULL);

for the ct-zone case set, ips_to_migrate as NULL/empty.

dump_conntrack(NULL, ct_zones_to_migrate);

void
init_lmct_config();

void
Copy link
Collaborator

Choose a reason for hiding this comment

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

on another note, let us move this to common.h and the corresponding implementation in common.c instead.

lmct_config is basically something which is read from the conf file and populated.
filter_type is read from args instead.

nfct_attr_is_set(ct, ATTR_ORIG_ZONE) > 0 ||
nfct_attr_is_set(ct, ATTR_REPL_ZONE) > 0) {
return false;
if(fltr_type == FILTER_BY_IP) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit space after if

is_entry_useful = is_src_or_dst_in_hashtable(src_addr, dst_addr,
ips_to_migrate);
} else {
zone = nfct_get_attr_u16(ct, ATTR_ZONE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

you need to first check if the zone is actually present in the entry.

@@ -280,19 +295,29 @@ conntrack_events_callback(const struct nlmsghdr *nlh, void *data)
return MNL_CB_OK;
}

update_conntrack_store(conn_store, ct, type);
bool add_to_conntrack_store = true;
if( fltr_type == FILTER_BY_CT_ZONE ){
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix spacing.

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.

2 participants