Skip to content
This repository has been archived by the owner on Sep 5, 2023. It is now read-only.

rpma: document default values of struct rpma_conn_cfg #1537

Merged
merged 1 commit into from
Feb 7, 2022

Conversation

Patryk717
Copy link
Contributor

@Patryk717 Patryk717 commented Feb 3, 2022

Ref: #978


This change is Reviewable

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @ldorau)

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ldorau and @Patryk717)

a discussion (no related file):
The CI builds failed, please check it locally before pushing it out


Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ldorau and @Patryk717)


-- commits, line 3 at r1:
There has to be exactly one empty line between commit's title and body.

Code quote:

- ce19957: rpma: Undocummented struct rpma_conn_cfg * defualt values
  Ref: #978

src/include/librpma.h, line 1212 at r1 (raw file):

 * DESCRIPTION
 * rpma_conn_cfg_new() creates a new connection configuration object.
 * when no *cfg is provided the default is used instead: https://pmem.io/rpma/manpages/master/rpma_conn_req_new.3

What does *cfg mean here?

Code quote:

*cfg

src/include/librpma.h, line 1213 at r1 (raw file):

 * rpma_conn_cfg_new() creates a new connection configuration object.
 * when no *cfg is provided the default is used instead: https://pmem.io/rpma/manpages/master/rpma_conn_req_new.3
 * when struct rpma_conn_cfg * is not modified it has the default values: https://pmem.io/rpma/manpages/master/rpma_conn_cfg_new.3  

This comment does not make sense here - The cfg is created here, not modified.

Code quote:

 * when struct rpma_conn_cfg * is not modified it has the default values: https://pmem.io/rpma/manpages/master/rpma_conn_cfg_new.3···

Copy link
Contributor Author

@Patryk717 Patryk717 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ldorau and @Patryk717)


-- commits, line 3 at r1:

Previously, ldorau (Lukasz Dorau) wrote…

There has to be exactly one empty line between commit's title and body.

Done.


src/include/librpma.h, line 1212 at r1 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

What does *cfg mean here?

this is my mistake


src/include/librpma.h, line 1213 at r1 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

This comment does not make sense here - The cfg is created here, not modified.

sure this is my mistake

Copy link
Contributor Author

@Patryk717 Patryk717 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ldorau)

a discussion (no related file):

Previously, ldorau (Lukasz Dorau) wrote…

The CI builds failed, please check it locally before pushing it out

The CI builds now work


Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Patryk717)


src/include/librpma.h, line 1211 at r2 (raw file):

 *
 * DESCRIPTION
 * rpma_conn_cfg_new() creates a new connection configuration object.

rpma_conn_cfg_new() creates a new connection configuration object and fills it with the default values: ... - please list the default values here.

Code quote:

rpma_conn_cfg_new() creates a new connection configuration object.

src/include/librpma.h, line 1275 at r2 (raw file):

 * DESCRIPTION
 * rpma_conn_cfg_set_timeout() sets the connection establishment timeout.
 * If timeout is not specified, it defaults to 1000.

It this function is not called, the timeout has the default value (1000) set by rpma_conn_cfg_new(3).

Code quote:

If timeout is not specified, it defaults to 1000.

src/include/librpma.h, line 1335 at r2 (raw file):

 * DESCRIPTION
 * rpma_conn_cfg_set_cq_size() sets the CQ size for the connection.
 * If cq_size not specified, it defaults to 10.

.
(change as above)

Code quote:

If cq_size not specified, it defaults to 10.

src/include/librpma.h, line 1397 at r2 (raw file):

 * rpma_conn_cfg_set_rcq_size() sets the receive CQ size for the connection.
 * Please see the rpma_conn_get_rcq() for details about the receive CQ.
 * If rcq_size not specified, it defaults to 0.

.


src/include/librpma.h, line 1459 at r2 (raw file):

 * DESCRIPTION
 * rpma_conn_cfg_set_sq_size() sets the SQ size for the connection.
 * If sq_size not specified, it defaults to 10.

.


src/include/librpma.h, line 1520 at r2 (raw file):

 * DESCRIPTION
 * rpma_conn_cfg_set_rq_size() sets the RQ size for the connection.
 * If rq_size not specified, it defaults to 10.

.

Copy link
Contributor Author

@Patryk717 Patryk717 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @ldorau)


src/include/librpma.h, line 1211 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

rpma_conn_cfg_new() creates a new connection configuration object and fills it with the default values: ... - please list the default values here.

Done.


src/include/librpma.h, line 1275 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

It this function is not called, the timeout has the default value (1000) set by rpma_conn_cfg_new(3).

Done.


src/include/librpma.h, line 1335 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.
(change as above)

done


src/include/librpma.h, line 1397 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.


src/include/librpma.h, line 1459 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.


src/include/librpma.h, line 1520 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Patryk717)


src/include/librpma.h, line 1280 at r3 (raw file):

 * DESCRIPTION
 * rpma_conn_cfg_set_timeout() sets the connection establishment timeout.
 * It this function is not called, the timeout has the default value (1000) set by rpma_conn_cfg_new(3).

If ...

Code quote:

It 

src/include/librpma.h, line 1340 at r3 (raw file):

 * DESCRIPTION
 * rpma_conn_cfg_set_cq_size() sets the CQ size for the connection.
 * It this function is not called, the cq_size has the default value (10) set by rpma_conn_cfg_new(3).

.


src/include/librpma.h, line 1402 at r3 (raw file):

 * rpma_conn_cfg_set_rcq_size() sets the receive CQ size for the connection.
 * Please see the rpma_conn_get_rcq() for details about the receive CQ.
 * It this function is not called, the rcq_size has the default value (0) set by rpma_conn_cfg_new(3).

.


src/include/librpma.h, line 1464 at r3 (raw file):

 * DESCRIPTION
 * rpma_conn_cfg_set_sq_size() sets the SQ size for the connection.
 * It this function is not called, the sq_size has the default value (10) set by rpma_conn_cfg_new(3).

.


src/include/librpma.h, line 1525 at r3 (raw file):

 * DESCRIPTION
 * rpma_conn_cfg_set_rq_size() sets the RQ size for the connection.
 * It this function is not called, the rq_size has the default value (10) set by rpma_conn_cfg_new(3).

.


src/include/librpma.h, line 1955 at r3 (raw file):

 * The rpma_conn_req_new() function returns 0 on success or a negative
 * error code on failure. rpma_conn_req_new() does not set
 * if cfg is not given it has default values
  1. Do not insert a new sentence in the middle of another one.
  2. Please change it to:
If cfg is NULL, then the default values are used - see rpma_conn_cfg_new(3) for more details.

Code quote:

if cfg is not given it has default values

@Patryk717 Patryk717 force-pushed the document_cfg branch 2 times, most recently from cf06bca to d390fd0 Compare February 4, 2022 13:33
Copy link
Contributor Author

@Patryk717 Patryk717 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @ldorau)


src/include/librpma.h, line 1280 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

If ...

Done.


src/include/librpma.h, line 1340 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.


src/include/librpma.h, line 1402 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.


src/include/librpma.h, line 1464 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.


src/include/librpma.h, line 1525 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.


src/include/librpma.h, line 1955 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…
  1. Do not insert a new sentence in the middle of another one.
  2. Please change it to:
If cfg is NULL, then the default values are used - see rpma_conn_cfg_new(3) for more details.

Done.

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Patryk717)


src/include/librpma.h, line 1212 at r4 (raw file):

 * DESCRIPTION
 * rpma_conn_cfg_new() creates a new connection configuration
 * object and fills it with the default values:

Please move object to the previous line as it was before.

Code quote:

object

Copy link
Contributor Author

@Patryk717 Patryk717 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ldorau)


src/include/librpma.h, line 1212 at r4 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Please move object to the previous line as it was before.

Done.

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Patryk717)

@ldorau ldorau changed the title rpma: Undocummented struct rpma_conn_cfg * defualt values rpma: document default values of struct rpma_conn_cfg Feb 4, 2022
Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Patryk717)


-- commits, line 2 at r6:

rpma: document default values of struct rpma_conn_cfg

Ref: #978 

Code quote:

rpma: Undocummented struct rpma_conn_cfg * defualt values

Copy link
Contributor Author

@Patryk717 Patryk717 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @ldorau)


-- commits, line 2 at r6:

Previously, ldorau (Lukasz Dorau) wrote…
rpma: document default values of struct rpma_conn_cfg

Ref: #978 

Done.

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Patryk717)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Patryk717)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

🐐

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Patryk717)

a discussion (no related file):
Shall we add this to CHANGELOG?


Copy link
Contributor

@osalyk osalyk left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Patryk717)


src/include/librpma.h, line 1213 at r7 (raw file):

 * rpma_conn_cfg_new() creates a new connection configuration object
 * and fills it with the default values:
 * .timeout_ms = 1000
  • timeout_ms = 1000
    ...

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk and @Patryk717)

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Shall we add this to CHANGELOG?

OK, @Patryk717 please add the following line:

- Documented the default values of struct rpma_conn_cfg.

after the line:

- Example (#12) for separate receive completion queue (RCQ).

so it should look like:

...
## [Unreleased]
### Added
- Example (#12) for separate receive completion queue (RCQ).
- Documented the default values of struct rpma_conn_cfg.

- APIs:
  - rpma_cq_get_wc - receive one or more completions

...

Code quote (from -- commits):

document default values of struct rpma_conn_cfg


src/include/librpma.h, line 1213 at r7 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…
  • timeout_ms = 1000
    ...

@osalyk What do you mean?

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk and @Patryk717)

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk and @Patryk717)


src/include/librpma.h, line 1213 at r7 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

@osalyk What do you mean?

Something like this?

 * - .timeout_ms = 1000
 * - .cq_size = 10
 * - .rcq_size = 0
 * - .sq_size = 10
 * - .rq_size = 10

Code quote:

 * .timeout_ms = 1000
 * .cq_size = 10
 * .rcq_size = 0
 * .sq_size = 10
 * .rq_size = 10

Copy link
Contributor

@osalyk osalyk left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ldorau and @Patryk717)


src/include/librpma.h, line 1213 at r7 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Something like this?

 * - .timeout_ms = 1000
 * - .cq_size = 10
 * - .rcq_size = 0
 * - .sq_size = 10
 * - .rq_size = 10

Exactly

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk and @Patryk717)


src/include/librpma.h, line 1213 at r7 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

Exactly

ok

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk and @Patryk717)

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk and @Patryk717)

a discussion (no related file):

Previously, ldorau (Lukasz Dorau) wrote…

OK, @Patryk717 please add the following line:

- Documented default values of struct rpma_conn_cfg.

after the line:

- Example (#12) for separate receive completion queue (RCQ).

so it should look like:

...
## [Unreleased]
### Added
- Example (#12) for separate receive completion queue (RCQ).
- Documented default values of struct rpma_conn_cfg.

- APIs:
  - rpma_cq_get_wc - receive one or more completions

...

- Documented the default values of struct rpma_conn_cfg.


Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk and @Patryk717)

Copy link
Contributor Author

@Patryk717 Patryk717 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @grom72, @ldorau, and @osalyk)

a discussion (no related file):

Previously, ldorau (Lukasz Dorau) wrote…

- Documented the default values of struct rpma_conn_cfg.

Done.



src/include/librpma.h, line 1213 at r7 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

ok

Done.

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)

Copy link
Contributor

@osalyk osalyk left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Patryk717)

Copy link
Member

@ldorau ldorau left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Patryk717)

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

Successfully merging this pull request may close these issues.

None yet

5 participants