Project

General

Profile

Actions
Copy link

Optimization #7186

open
JF P

detect: represent direction with enum

Optimization #7186: detect: represent direction with enum

Added by Juliana Fajardini Reichow about 2 years ago. Updated 4 days ago.

Status:
Assigned
Priority:
Normal
Assignee:
Paul Lege
Target version:
9.0.0-beta1
Effort:
Difficulty:
Label:
Beginner, C, Good First Issue

Description

As per the discussion seen in https://github.com/OISF/suricata/pull/11530#discussion_r1690185187, replace uint8_t with an enum to represent app-proto direction (https://github.com/OISF/suricata/blob/master/src/detect.h#L431).

PA Updated by Philippe Antoine about 2 years ago Actions
Copy link
 #1

  • Label Beginner, C, Good First Issue added

I would say a bool rather than an enum with 2 values

JF Updated by Juliana Fajardini Reichow about 2 years ago 1Actions
Copy link
 #2

Philippe Antoine wrote in #note-1:

I would say a bool rather than an enum with 2 values

What I'd like to achieve with this is more readability - as well as type-checking if possible. As long as we have that, it's good, imho.

VJ Updated by Victor Julien almost 2 years ago Actions
Copy link
 #3

  • Target version changed from 8.0.0-beta1 to 9.0.0-beta1

PA Updated by Philippe Antoine 8 months ago Actions
Copy link
 #4

  • Status changed from New to Assigned

PA Updated by Philippe Antoine 3 months ago Actions
Copy link
 #5

  • Status changed from Assigned to Triaged

VJ Updated by Victor Julien 8 days ago 1Actions
Copy link
 #6

I don't like the bool for this, as it will always force you to think about what true and false map to.

JF Updated by Juliana Fajardini Reichow 7 days ago Actions
Copy link
 #7

  • Assignee changed from OISF Dev to Community Ticket

P Updated by Paul Lege 7 days ago Actions
Copy link
 #8

  • Assignee changed from Community Ticket to Paul Lege

JF Updated by Juliana Fajardini Reichow 6 days ago Actions
Copy link
 #9

  • Status changed from Triaged to Assigned

P Updated by Paul Lege 4 days ago Actions
Copy link
 #10

The purpose of this ticket is to change the type of the “dir” field in the DetectEngineAppInspectionEngine structure.

Should this enum support assignments for both directions (it currently does not, but this was one of the requests in the original GitHub thread)?

Does the scope of this ticket extend only to DetectEngineAppInspectionEngine, or is the enum intended for use in other structures that store direction information?

Furthermore, converting from a 'uint8_t' type to an 'enum' changes the memory size of the field (which isn’t an issue here since there was padding in the structure). C23 supports enum underlying type changes. Are we in agreement that Suricata does not officially support this version of the language? I don’t want to create a macro just to use this feature if it goes against the codebase (from what I’ve read in the build system, the supported version is C11 and C99, but since I couldn’t find this information in the documentation, I’d rather ask).

Actions
Copy link

Also available in: PDF Atom