Skip to content

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Jun 3, 2020

Fixes #1309

Contents:

  1. Added management for a configuration object. The configuration object is dynamically created (there's unfortunately no other way in C API). After the connection process those objects should be deleted (deletion can be done anyway and on a NULL object it will simply do nothing). The configuration object is a container that contains options to be set on the socket prior to connecting. These options apply AFTER the options derived through the group are set.

  2. The testing application is now capable of specifying single socket options using srto. prefix attached to the node specification in an argument-separate syntax.

@ethouris ethouris added Priority: High Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core Impact: Significant labels Jun 3, 2020
@ethouris ethouris added this to the v1.5.0 milestone Jun 3, 2020
@ethouris ethouris self-assigned this Jun 3, 2020
@maxsharabayko maxsharabayko mentioned this pull request Jun 3, 2020
33 tasks
Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

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

In general looks good. This would give the flexibility in configuring each socket in a group.

The folowing API functions are added:

  • srt_create_config(..)
  • srt_delete_config(..)
  • srt_config_add(..)
  1. Maybe some consestent naming could be used? srt_config_*(..)? Although there is the same with the group API naming.
  2. srt_config_remove(..) could be useful as well, but I guess it can be easily added later on request.

@maxsharabayko
Copy link
Collaborator

It might be good to create a separate source files for the configuration object.
core.cpp keeps growing.

@ethouris
Copy link
Collaborator Author

ethouris commented Jun 3, 2020

I understand the necessity to trim out the fat of core.cpp, but this better be done as a planned initiative, not ad-hoc.

@maxsharabayko maxsharabayko modified the milestones: v1.5.0, v1.5.0 - Sprint 16 Jun 4, 2020
…n result. Changed all-fail result to CONNLOST.
@ethouris ethouris marked this pull request as ready for review June 5, 2020 14:16
@ethouris ethouris changed the title Reworked group status and connection configuration Added configuration object to group connection structure Jun 8, 2020
@maxsharabayko
Copy link
Collaborator

Confirming the idea to have srt_create_config and srt_delete_config to create and delete an object, while srt_config_add adds entry to the object.
This looks aligned to the existing API:

  • srt_setsockopt(..)
  • srt_create_socket(..)

@maxsharabayko
Copy link
Collaborator

After the connection process only those objects should be deleted that weren't reset to NULL.

I think the deletion of the object should be done by the upstream app.
This is the mission of srt_delete_config(..).

@ethouris
Copy link
Collaborator Author

ethouris commented Jun 9, 2020

The idea is that the configuration object should only be deleted in case when you decided that you don't need to make a connection. Don't forget that this is a C API and any simplification here will be welcome.

In short: you need to perform deletion only if this object has been created, but you are somehow unable to call the srt_connect_group function. If you know prematurely that you WILL pass this object to the structure passed to this function, you don't have to delete it at all. This function is simply provided for completion because we cannot leave a user with no possibility to delete an object and just sentenced to call the connect function anyway, or live with a leak.

Just as an additional help, pointers to objects that have been "eaten up" by the connection function are reset to NULL. So the user should simply fill the config field with the result of srt_create_config() directly, and call srt_delete_config() on all of them, if they aren't certain if they called srt_connect_group().

@maxsharabayko
Copy link
Collaborator

Usage example

SRT_SOCKGROUPCONFIG data [] = {
    srt_prepare_endpoint( ... 1 ... ),
    srt_perpare_endpoint( ... 2 ... )
};

data[0].config = srt_create_config(); 
srt_config_add(data[0].config, SRT_BINDTODEVICE, "eth0", 4);

data[1].config = srt_create_config();
srt_config_add(data[1].config, SRT_BINDTODEVICE, "eth1", 4);

size_t len = sizeof data;
srt_connect_group(grp, data, len);

// if srt_connect_group suceeded, then data[i].config will be nullptr
for (auto& d: data)
    srt_delete_config(d.config);

@maxsharabayko
Copy link
Collaborator

Sharing the same config is prohibited, and will cause a crash(?)

SRT_SOCKGROUPCONFIG data [] = {
    srt_prepare_endpoint( ... 1 ... ),
    srt_perpare_endpoint( ... 2 ... )
};

SRT_SOCKOPT_CONFIG* config = srt_create_config();
srt_config_add(config, SRT_BINDTODEVICE, "eth0", sizeof int);

data[0].config = config;
data[1].config = config;

size_t len = sizeof data;
// Crash on deleting already deleted obect
srt_connect_group(grp, data, len);

// if srt_connect_group suceeded, then data[i].config will be nullptr
for (auto& d: data)
    srt_delete_config(d.config);

@ethouris
Copy link
Collaborator Author

ethouris commented Jun 9, 2020

In the current proposal, IF srt_connect_group was called (no matter if succeeded or failed), objects will be deleted. Manual deletion would be only necessary if - by some reason - you created the object, but you didn't call srt_connect_group.

Copy link
Collaborator

@maxsharabayko maxsharabayko left a comment

Choose a reason for hiding this comment

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

Unanonimous decision: the srt_connect_group should not delete the data.
Config object is to be deleted only by the app via srt_delete_config.
Requesting changes.

@maxsharabayko maxsharabayko merged commit e03dbc6 into Haivision:master Jun 11, 2020
@mbakholdina mbakholdina modified the milestones: v1.5.0 - Sprint 16, v1.4.2 Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Priority: High Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR] Bonding API: Add a way to retrieve socket group member link status
3 participants