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

examples: add common-pmem_map_file and signature_check to example09 #1909

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

haichangsi
Copy link
Contributor

@haichangsi haichangsi commented Jul 1, 2022

This change is Reviewable

@haichangsi haichangsi requested review from grom72 and osalyk July 1, 2022 10:58
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 3 of 4 files at r1.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @haichangsi and @osalyk)


examples/09-flush-to-persistent-GPSPM/CMakeLists.txt line 43 at r1 (raw file):

link_directories(${LIBRPMA_LIBRARY_DIRS})

function(add_example name)

have not we talked already to move this function to a common location?

Code quote:

function(add_example name)

examples/09-flush-to-persistent-GPSPM/client.c line 301 at r1 (raw file):

#ifdef USE_PMEM
	if (mem.is_pmem) {
		common_pmem_unmap_file(&mem);

Suggestion:

#ifdef USE_PMEM
	if (mem.is_pmem) {
		common_pmem_unmap_file(&mem);
	} else
#endif /* USE_PMEM */

examples/09-flush-to-persistent-GPSPM/server.c line 244 at r1 (raw file):

#else
	(void) printf(
			"At this point, persist function should be called if libpmem2 or libpmem will be in use\n");

Suggestion:

persistent memory

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 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @haichangsi and @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.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @grom72 and @haichangsi)


examples/09-flush-to-persistent-GPSPM/CMakeLists.txt line 43 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

have not we talked already to move this function to a common location?

+1 add_example_with_pmem()


examples/09-flush-to-persistent-GPSPM/client.c line 134 at r1 (raw file):

	if ((ret = rpma_mr_reg(peer, msg_ptr, KILOBYTE,
			RPMA_MR_USAGE_SEND | RPMA_MR_USAGE_RECV,
			&msg_mr))) {

if ((ret = rpma_mr_reg(peer, msg_ptr, KILOBYTE, RPMA_MR_USAGE_SEND | RPMA_MR_USAGE_RECV,
&msg_mr))) {

Please correct all similar cases

Code quote:

	if ((ret = rpma_mr_reg(peer, msg_ptr, KILOBYTE,
			RPMA_MR_USAGE_SEND | RPMA_MR_USAGE_RECV,
			&msg_mr))) {

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 4 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @grom72 and @haichangsi)

Copy link
Contributor Author

@haichangsi haichangsi 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: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @grom72 and @osalyk)


examples/09-flush-to-persistent-GPSPM/CMakeLists.txt line 43 at r1 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

+1 add_example_with_pmem()

Done.


examples/09-flush-to-persistent-GPSPM/client.c line 134 at r1 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

if ((ret = rpma_mr_reg(peer, msg_ptr, KILOBYTE, RPMA_MR_USAGE_SEND | RPMA_MR_USAGE_RECV,
&msg_mr))) {

Please correct all similar cases

Done.


examples/09-flush-to-persistent-GPSPM/client.c line 301 at r1 (raw file):

#ifdef USE_PMEM
	if (mem.is_pmem) {
		common_pmem_unmap_file(&mem);

Done.


examples/09-flush-to-persistent-GPSPM/server.c line 244 at r1 (raw file):

#else
	(void) printf(
			"At this point, persist function should be called if libpmem2 or libpmem will be in use\n");

Done.

@haichangsi haichangsi force-pushed the example09 branch 2 times, most recently from 7d91611 to b2cf641 Compare July 5, 2022 07:16
Copy link
Contributor Author

@haichangsi haichangsi 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: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @grom72, @ldorau, and @osalyk)


examples/09-flush-to-persistent-GPSPM/CMakeLists.txt line 43 at r1 (raw file):

Previously, haichangsi (Haichang) wrote…

Done.

no, you are wrong, as we discussed with @ldorau in this example we use libprotobuf so I had to leave it the way it is

Copy link
Contributor Author

@haichangsi haichangsi left a comment

Choose a reason for hiding this comment

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

Ref: #1791

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

@haichangsi haichangsi force-pushed the example09 branch 2 times, most recently from ecfa925 to 71734ce Compare July 5, 2022 09:27
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 2 of 2 files at r3, 1 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @haichangsi and @osalyk)


examples/09-flush-to-persistent-GPSPM/CMakeLists.txt line 44 at r5 (raw file):

link_directories(${LIBRPMA_LIBRARY_DIRS})

add_example_with_pmem(server server.c GPSPM_flush.pb-c.c ../common/common-conn.c)

what about options USE_LIBPROTOBUFC?

Suggestion:

add_example_with_pmem(server server.c GPSPM_flush.pb-c.c ../common/common-conn.c USE_LIBPROTOBUFC)
add_example_with_pmem(client client.c GPSPM_flush.pb-c.c ../common/common-conn.c USE_LIBPROTOBUFC)

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 all commit messages.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @grom72 and @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.

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

Copy link
Contributor Author

@haichangsi haichangsi 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)


examples/09-flush-to-persistent-GPSPM/CMakeLists.txt line 44 at r5 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

what about options USE_LIBPROTOBUFC?

It is a bigger problem, I've tried many solutions on the second branch, none of them works

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 @osalyk)


examples/09-flush-to-persistent-GPSPM/CMakeLists.txt line 44 at r5 (raw file):

Previously, haichangsi (Haichang) wrote…

It is a bigger problem, I've tried many solutions on the second branch, none of them works

Let's move to intersprint

@grom72 grom72 added the inter-sprint Between-sprints activity label Jul 8, 2022
@ldorau ldorau requested a review from osalyk July 18, 2022 10:53
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, 3 unresolved discussions (waiting on @haichangsi and @osalyk)


examples/09-flush-to-persistent-GPSPM/CMakeLists.txt line 44 at r6 (raw file):

link_directories(${LIBRPMA_LIBRARY_DIRS})

add_example_with_pmem(server server.c GPSPM_flush.pb-c.c ../common/common-conn.c USE_LIBPROTOBUFC)

USE_LIBPROTOBUFC is not a source file

Code quote:

USE_LIBPROTOBUFC

examples/09-flush-to-persistent-GPSPM/CMakeLists.txt line 45 at r6 (raw file):

add_example_with_pmem(server server.c GPSPM_flush.pb-c.c ../common/common-conn.c USE_LIBPROTOBUFC)
add_example_with_pmem(client client.c GPSPM_flush.pb-c.c ../common/common-conn.c USE_LIBPROTOBUFC)

.

Code quote:

USE_LIBPROTOBUFC

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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @haichangsi and @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, 3 unresolved discussions (waiting on @haichangsi and @osalyk)


examples/09-flush-to-persistent-GPSPM/CMakeLists.txt line 44 at r6 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

USE_LIBPROTOBUFC is not a source file

It seems that add_example_with_pmem() should be fixed.

Code quote:

add_example_with_pmem

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

Copy link
Contributor Author

@haichangsi haichangsi 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: 3 of 8 files reviewed, 2 unresolved discussions (waiting on @grom72, @ldorau, and @osalyk)


examples/09-flush-to-persistent-GPSPM/CMakeLists.txt line 44 at r6 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

It seems that add_example_with_pmem() should be fixed.

Done.


examples/09-flush-to-persistent-GPSPM/CMakeLists.txt line 45 at r6 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

.

Done.

@codecov
Copy link

codecov bot commented Jul 19, 2022

Codecov Report

Merging #1909 (4a18687) into master (b252e55) will not change coverage.
The diff coverage is n/a.

❗ Current head 4a18687 differs from pull request most recent head 025a67f. Consider uploading reports for the commit 025a67f to get more accurate results

@@            Coverage Diff            @@
##            master     #1909   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines         1425      1424    -1     
=========================================
- Hits          1425      1424    -1     

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 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ldorau)

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:

Reviewable status: all files reviewed, 2 unresolved discussions (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.

Reviewed 1 of 4 files at r1, 2 of 2 files at r3, 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @haichangsi)

a discussion (no related file):
The first commit does not build:
https://github.com/ldorau/rpma/actions/runs/2697237557

Each and every commit has to build correctly.
You should not submit one commit with bugs and the next one with fixes.
Squash them please.


Copy link
Contributor Author

@haichangsi haichangsi 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)

a discussion (no related file):

Previously, ldorau (Lukasz Dorau) wrote…

The first commit does not build:
https://github.com/ldorau/rpma/actions/runs/2697237557

Each and every commit has to build correctly.
You should not submit one commit with bugs and the next one with fixes.
Squash them please.

I know. In the beggining it was like that:
1st one - changes to both client and server (bulding correctly)
2nd - cmake changes (now also building correctly)
Maybe during this whole process sth has changed. Sorry. 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: all files reviewed, 1 unresolved discussion (waiting on @haichangsi)


-- commits line 4 at r8:
use capital letters

Code quote:

use_libprotobufc

Move USE_LIBPROTOBUFC to the common.cmake file.
Copy link
Contributor Author

@haichangsi haichangsi 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 4 at r8:

Previously, ldorau (Lukasz Dorau) wrote…

use capital letters

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 @haichangsi)

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @haichangsi)

@ldorau ldorau merged commit c0147c2 into pmem:master Jul 19, 2022
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.

4 participants