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

Update all examples to use new APIs #1271

Merged
merged 9 commits into from
Oct 1, 2021
Merged

Conversation

yangx-jy
Copy link
Contributor

@yangx-jy yangx-jy commented Sep 9, 2021

Ref: #1212


This change is Reviewable

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 12 of 12 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)

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 12 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)

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 8 of 12 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)

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 4 of 12 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @yangx-jy)


examples/06-multiple-connections/server.c, line 153 at r1 (raw file):

/* get the connection's completion fd and add it to epoll */

This comment should be moved before rpma_cq_get_fd(cq, &fd); and a new one should be added here.


examples/06-multiple-connections/server.c, line 216 at r1 (raw file):

/* prepare detected completions for processing */

wrong comment


examples/06-multiple-connections/server.c, line 220 at r1 (raw file):

/* another error occurred - disconnect */

.

Copy link
Contributor Author

@yangx-jy yangx-jy 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: 11 of 12 files reviewed, 3 unresolved discussions (waiting on @grom72 and @ldorau)


examples/06-multiple-connections/server.c, line 153 at r1 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…
/* get the connection's completion fd and add it to epoll */

This comment should be moved before rpma_cq_get_fd(cq, &fd); and a new one should be added here.

Done.


examples/06-multiple-connections/server.c, line 216 at r1 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…
/* prepare detected completions for processing */

wrong comment

Done.


examples/06-multiple-connections/server.c, line 220 at r1 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…
/* another error occurred - disconnect */

.

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.

:lgtm:

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

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 r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)

Copy link

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 12 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yangx-jy)


examples/06-multiple-connections/server.c, line 401 at r2 (raw file):

		 */
		return;
	}

I think you can add this part here:

	/* get the connection's main CQ */
	struct rpma_cq *cq = NULL;
	int ret = rpma_conn_get_cq(clnt->conn, &clnt->cq);
	if (ret) {
		/* an error occurred - disconnect */
		(void) rpma_conn_disconnect(clnt->conn);
		return;
	}

so, you don't need to call _get_cq() over and over again.

NOTE: You have to extend the struct client_res structure to accommodate also the cq.


examples/08-messages-ping-pong/client.c, line 131 at r2 (raw file):

					break;
				} else if ((ret = rpma_cq_get_completion(cq,
						&cmpl))) {

Won't it fit into a single line?

Copy link
Contributor Author

@yangx-jy yangx-jy 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: 11 of 12 files reviewed, 2 unresolved discussions (waiting on @grom72, @janekmi, and @ldorau)


examples/06-multiple-connections/server.c, line 401 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think you can add this part here:

	/* get the connection's main CQ */
	struct rpma_cq *cq = NULL;
	int ret = rpma_conn_get_cq(clnt->conn, &clnt->cq);
	if (ret) {
		/* an error occurred - disconnect */
		(void) rpma_conn_disconnect(clnt->conn);
		return;
	}

so, you don't need to call _get_cq() over and over again.

NOTE: You have to extend the struct client_res structure to accommodate also the cq.

Done. Good suggestion. ^_^


examples/08-messages-ping-pong/client.c, line 131 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Won't it fit into a single line?

Yes, make cstyle will show line > 80 characters when it is changed to a single line.

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 r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

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, 2 unresolved discussions (waiting on @janekmi)

Copy link

@janekmi janekmi 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 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yangx-jy)

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 @yangx-jy)

@janekmi janekmi merged commit ad99e02 into pmem:master Oct 1, 2021
Patryk717 pushed a commit to Patryk717/rpma that referenced this pull request Jan 20, 2022
Patryk717 pushed a commit to Patryk717/rpma that referenced this pull request Jan 20, 2022
Patryk717 pushed a commit to Patryk717/rpma that referenced this pull request Jan 20, 2022
Patryk717 pushed a commit to Patryk717/rpma that referenced this pull request Jan 20, 2022
Patryk717 pushed a commit to Patryk717/rpma that referenced this pull request Jan 21, 2022
Patryk717 pushed a commit to Patryk717/rpma that referenced this pull request Jan 21, 2022
Patryk717 pushed a commit to Patryk717/rpma that referenced this pull request Jan 24, 2022
Patryk717 pushed a commit to Patryk717/rpma that referenced this pull request Jan 24, 2022
Patryk717 pushed a commit to Patryk717/rpma that referenced this pull request Jan 24, 2022
ldorau pushed a commit to ldorau/rpma that referenced this pull request Jan 24, 2022
Patryk717 pushed a commit to Patryk717/rpma that referenced this pull request Jan 24, 2022
ldorau pushed a commit to ldorau/rpma that referenced this pull request Jan 24, 2022
Patryk717 pushed a commit to Patryk717/rpma that referenced this pull request Jan 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants