Skip to content

Conversation

ldegio
Copy link
Contributor

@ldegio ldegio commented Oct 30, 2018

added the ability to specify a set of ports where data is captured with bigger snaplen (20000)

Copy link
Contributor

@gianlucaborello gianlucaborello left a comment

Choose a reason for hiding this comment

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

Some minor comments, nothing is major except the initialization part in BPF, so just that one needs to be addressed, the others are a best effort.

/*
* mpegts detection
*/
return 16000;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: eventually move this to a define since it's shared between bpf and driver (wasn't done for the other part of the function as part of the initial porting because I didn't want to cause a large diff in the driver code). Not a deal breaker considering the current status.

//
if(handle->m_mode != SCAP_MODE_LIVE)
{
snprintf(handle->m_lasterr, SCAP_LASTERR_SIZE, "scap_set_fullcapture_port_range not supported on this scap mode");
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: there seems to be a spurious tab instead of a whitespace.

throw sinsp_exception("set_fullcapture_port_range called before capture start");
}

if(is_live() && scap_set_fullcapture_port_range(m_h, range_start, range_end) != SCAP_SUCCESS)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we call this function on an offline inspector we don't get an error, the call silently ignores everything, is that expected?

Perhaps is_live() could be moved to a separate check and throw an exception if false, or removed altogether and let scap return error if opened on a trace file.

@@ -206,6 +206,8 @@ struct sysdig_bpf_settings {
bool dropping_mode;
bool is_dropping;
bool tracers_enabled;
uint16_t fullcapture_port_range_start;
uint16_t fullcapture_port_range_end;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like these values are never initialized in scap. BPF maps are not automatically cleared, so we essentially get garbage by default. Please look at the set_default_settings function in scap_bpf.c.

ldegio and others added 7 commits November 8, 2018 16:33
We're using the wrong placeholders in some calls to printf-like
functions.  This problem was identified when we started building
with -Wextra.  This change use %zu for size_t, PRIu64 for uint64_t
and PRId64 for int64_t.
@ldegio ldegio merged commit 9722dbc into dev Dec 21, 2018
thom-sd pushed a commit that referenced this pull request Dec 21, 2018
* added the ability to specify a set of ports where data is captured with bigger snaplen (20000)

* fixes based on gianluca's review

* new chisel: udp_extract

* Fix snprintf placeholder for size_t/{u,}int64_t (#1279)

We're using the wrong placeholders in some calls to printf-like
functions.  This problem was identified when we started building
with -Wextra.  This change use %zu for size_t, PRIu64 for uint64_t
and PRId64 for int64_t.
@ldegio ldegio deleted the mpegts branch April 13, 2022 21:31
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.

3 participants