split rsaperf into its low and high level parts

ASSIGNED
Assigned to

Status

NSS
Tools
ASSIGNED
5 years ago
5 years ago

People

(Reporter: Elio Maldonado, Assigned: Elio Maldonado)

Tracking

trunk
3.14.3
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

5 years ago
A freebl-only spin off from rsaperf is needed to go with the nss-softokn builds on fedora and recent versions of RHEL and its counterpart the high level only spin off of rsaperf.
(Assignee)

Comment 1

5 years ago
This request is to support ongoing work in Fedora to build nss withou any softoken in the tree. In such builds the tools are linked against the softoken and freebl libraries in the build environment instead of the ones in the build tree. This way when we run the tests we are testing the combinations that actually reflect what is going to be shipped to customers. On RHEL softoken/freebl may be of an earlier version - the last one to receive FIPS 140 validation.

This request is to enable to fedora/rehel nss downtsream maintainers to build nss and nss-softoken using such split versions of rsaperf and does not necessarily mean we should abandon the traditional rsaperf. Besides the split versions I will submit patches to make building such new version conditional on some build environment variables so by default we build the traditional way.
(Assignee)

Updated

5 years ago
Target Milestone: --- → 3.14.2
(Assignee)

Comment 2

5 years ago
Created attachment 690642 [details] [diff] [review]
freebl-only spin-off from rsaperf

Patch written by Bob Relyea.
(Assignee)

Updated

5 years ago
Attachment #690642 - Attachment description: freebl-only spi- off from rsaperf → freebl-only spin-off from rsaperf
(Assignee)

Comment 3

5 years ago
Created attachment 690646 [details] [diff] [review]
high-level only spin-off from rsaperf

The counterpart to the low level one, written as a modified version as rsaperf.
(Assignee)

Updated

5 years ago
Assignee: nobody → emaldona
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

5 years ago
Created attachment 691009 [details] [diff] [review]
split rsaperf into low and high level components

Full patch, a bit conservative and it doesn't remove the old rsaperf.
Attachment #690642 - Attachment is obsolete: true
Attachment #690646 - Attachment is obsolete: true
Attachment #691009 - Flags: superreview?(wtc)
Attachment #691009 - Flags: review?(rrelyea)
(Assignee)

Comment 5

5 years ago
Created attachment 691010 [details] [diff] [review]
split rsaperf into low and high level components

A bit conservative as it doesn't remove the original rsaperf.
Attachment #691010 - Flags: review?(rrelyea)
(Assignee)

Updated

5 years ago
Attachment #691010 - Flags: review?(wtc)

Comment 6

5 years ago
Comment on attachment 691009 [details] [diff] [review]
split rsaperf into low and high level components

r-...

1) remove defkey_high.c. You don't need it. (it now has nothing but statics in it.so nothing in rsaperf_high.c depends on it).
2) remove the useBL symbol altogether.
3) In if (nickname & strcmp(nickname, "none")) {, you need an else. It should do either:

exec rsaperf_low with the arguments passed in (look at NSPR for the correct cross platform spellings, it's probably something like PR_System()).

 or 

Usage(prog);

I prefer the former.

4) please scour the argument list and remove the rsapref_low specific arguments when parsing (you can still accept them in case you have a -n none and you've implemented the exec rsaperf_low.

I would prefer rsaperf_high to become rsaperf, which execs rsaperf_low if gets the -n none rather than having 3 functions.

bob
Attachment #691009 - Flags: review?(rrelyea) → review-

Comment 7

5 years ago
Comment on attachment 691010 [details] [diff] [review]
split rsaperf into low and high level components

how is this different from your other patch?
Attachment #691010 - Flags: review?(rrelyea) → review-

Comment 8

5 years ago
BTW comments 6 2-4 refers to rsaperf_high.c

Of course rsaperf_low.c is perfect;) (see comment 2).... unless wtc says it's not;).

bob
(Assignee)

Comment 9

5 years ago
(In reply to Robert Relyea from comment #6)
> I would prefer rsaperf_high to become rsaperf, which execs rsaperf_low if
> gets the -n none rather than having 3 functions.
> 
Thank you, I very much like your suggestion. Having three functions to maintain and keep in sync. was something that bothered me. Your approach solves this. And no stinking conditionals as with an exec the CK_BYTE versus unsigned char is mute.
(Assignee)

Comment 10

5 years ago
Created attachment 692758 [details] [diff] [review]
split rsaperf into loe and high level parts

rsaperf "executes" rsaperf_low to do the freebl-only performance tests when the nickname is "none".
Attachment #691009 - Attachment is obsolete: true
Attachment #691010 - Attachment is obsolete: true
Attachment #691009 - Flags: superreview?(wtc)
Attachment #691010 - Flags: review?(wtc)
Attachment #692758 - Flags: review?(rrelyea)
(Assignee)

Updated

5 years ago
Attachment #692758 - Flags: review?(wtc)
(Assignee)

Updated

5 years ago
Summary: split rsaperf into its low and level parts → split rsaperf into its low and high level parts
(Assignee)

Comment 11

5 years ago
Created attachment 692782 [details] [diff] [review]
split rsaperf into low and high level components
Attachment #692758 - Attachment is obsolete: true
Attachment #692758 - Flags: review?(wtc)
Attachment #692758 - Flags: review?(rrelyea)
Attachment #692782 - Flags: superreview?(wtc)
Attachment #692782 - Flags: review?(rrelyea)

Comment 12

5 years ago
Comment on attachment 692782 [details] [diff] [review]
split rsaperf into low and high level components

r- defkey.c is still there...
Attachment #692782 - Flags: review?(rrelyea) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 696213 [details] [diff] [review]
split rsaperf into low and high level components
Attachment #692782 - Attachment is obsolete: true
Attachment #692782 - Flags: superreview?(wtc)
Attachment #696213 - Flags: review?(wtc)
(Assignee)

Updated

5 years ago
Attachment #696213 - Flags: review?(rrelyea)

Comment 14

5 years ago
Comment on attachment 696213 [details] [diff] [review]
split rsaperf into low and high level components

r-

1. In platlibs.mk, line 199: I would prefer if you dropped the OS_ARCH == Linux. The SOFTOKEN_LIB_DIR and NSS_USE_SYSTEM_FREEBL should be sufficient here.

2. You don't need lowArgs_count. Just use argc.

3. You don't really need to create a copy argVect. It's identical to argv except argv[0]. I'm OK with doing it for paranoia, but in your case you end up restricting the arg count (except you don't actually, you just go ahead and overwrite the stack).

4. PR_CreateProcess (and PR_Access for that matter) require full paths to work. You will need to find the patch to prognameLow for this all to work. (Best bet is to look in the directory rsaperf was executed in.

2 and 4 are the biggest things I want to have fixed. I can be talked out of 1 and 3.

bob
Attachment #696213 - Flags: review?(rrelyea) → review-
(Assignee)

Comment 15

5 years ago
(In reply to Robert Relyea from comment #14)
> Comment on attachment 696213 [details] [diff] [review]

> 1. In platlibs.mk, line 199: I would prefer if you dropped the OS_ARCH ==
> Linux. The SOFTOKEN_LIB_DIR and NSS_USE_SYSTEM_FREEBL should be sufficient
> here.

Yes, that should be sufficient.

> 
> 2. You don't need lowArgs_count. Just use argc.

Good
> 
> 3. You don't really need to create a copy argVect. It's identical to argv
> except argv[0]. I'm OK with doing it for paranoia, but in your case you end
> up restricting the arg count (except you don't actually, you just go ahead
> and overwrite the stack).

SAgrred
> 
> 4. PR_CreateProcess (and PR_Access for that matter) require full paths to
> work. You will need to find the patch to prognameLow for this all to work.
> (Best bet is to look in the directory rsaperf was executed in.

The only way I could do it was by finding the current directory using 
char *cwd = PR_GetInfo("PWD") and that requires rsaperf and rsaperf_low to be in the current working directory. From there I build the full path which is a bit long as you will see when I attach the revised patch. I couldn't find any other way.
> 
> 2 and 4 are the biggest things I want to have fixed. I can be talked out of
> 1 and 3.
Items 1 and 3 are welcome and so is 2, item 4 that made the code longer but for good reason.
(Assignee)

Comment 16

5 years ago
Created attachment 699935 [details] [diff] [review]
split rsaperf into low and high level components

Addresses the issues brought up by Bob. This is mostly to show some progress and get advise.
Attachment #699935 - Flags: review?(wtc)
Attachment #699935 - Flags: review?(rrelyea)
(Assignee)

Updated

5 years ago
Attachment #696213 - Attachment is obsolete: true
Attachment #696213 - Flags: review?(wtc)
(Assignee)

Comment 17

5 years ago
Comment on attachment 699935 [details] [diff] [review]
split rsaperf into low and high level components

That's the wrong patch.
Attachment #699935 - Attachment is obsolete: true
Attachment #699935 - Flags: review?(wtc)
Attachment #699935 - Flags: review?(rrelyea)

Comment 18

5 years ago
> The only way I could do it was by finding the current directory using 
> char *cwd = PR_GetInfo("PWD") and that requires rsaperf and rsaperf_low to be in the current 
> working directory.

Getting the current working directory doesn't really help, and the requirement that the tool be in the current working directory is too restrictive (It's almost never the case).

I don't think there is a simple solution to this. I would suggest at least scanning the the PATH environment variable looking for 'argv[0]'. You can also check argv[0] and see if it already has the full path (on some platforms it will). It may be PR_GetLibraryFilePathname() may work on the executible as well.

bob
(Assignee)

Updated

5 years ago
Target Milestone: 3.14.2 → 3.14.3
(Assignee)

Comment 19

5 years ago
Originally, I intended to solve Bug 820207 first and make it a blocker of this one but there are some issues of portability I must resolve there. This one on the other hand I think can stand on it's own.
(Assignee)

Comment 20

5 years ago
(In reply to Elio Maldonado from comment #19)
oops, that comment was intended for Bug 835919.
(Assignee)

Comment 21

5 years ago
Created attachment 747781 [details] [diff] [review]
split rsaperf into low and high level tools
(Assignee)

Comment 22

5 years ago
Created attachment 748593 [details] [diff] [review]
split rsaperf into now and high level components
Attachment #747781 - Attachment is obsolete: true
Attachment #748593 - Flags: review?(wtc)
Attachment #748593 - Flags: review?(rrelyea)
(Assignee)

Comment 23

5 years ago
Comment on attachment 748593 [details] [diff] [review]
split rsaperf into now and high level components

Cancelling, I must submit a new patch. This one breaks on Windows because of

+#ifdef XP_UNIX
+/* Determines, the best it can, whether the given path is a symbloic link
+ * and resolves it to the target. 
...
+ */
+actualPathIfSymLink(const char *programPath)
+{
....
+}
+#else
+/* TODO: Find a way to handle this on other platforms */
+#error "write actualPathIfSymLink for this platform"
+#endif
..............

A function I don't plan to use after all.
Attachment #748593 - Flags: review?(wtc)
Attachment #748593 - Flags: review?(rrelyea)
You need to log in before you can comment on or make changes to this bug.