Closed Bug 820207 Opened 12 years ago Closed 5 months ago

split rsaperf into its low and high level parts

Categories

(NSS :: Tools, enhancement, P5)

x86
Linux
enhancement

Tracking

(Not tracked)

RESOLVED WONTFIX
3.14.3

People

(Reporter: elio.maldonado.batiz, Unassigned)

References

Details

Attachments

(1 file, 9 obsolete files)

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.
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.
Target Milestone: --- → 3.14.2
Patch written by Bob Relyea.
Attachment #690642 - Attachment description: freebl-only spi- off from rsaperf → freebl-only spin-off from rsaperf
The counterpart to the low level one, written as a modified version as rsaperf.
Assignee: nobody → emaldona
Status: NEW → ASSIGNED
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)
A bit conservative as it doesn't remove the original rsaperf.
Attachment #691010 - Flags: review?(rrelyea)
Attachment #691010 - Flags: review?(wtc)
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 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-
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
(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.
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)
Attachment #692758 - Flags: review?(wtc)
Summary: split rsaperf into its low and level parts → split rsaperf into its low and high level parts
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 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-
Attachment #692782 - Attachment is obsolete: true
Attachment #692782 - Flags: superreview?(wtc)
Attachment #696213 - Flags: review?(wtc)
Attachment #696213 - Flags: review?(rrelyea)
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-
(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.
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)
Attachment #696213 - Attachment is obsolete: true
Attachment #696213 - Flags: review?(wtc)
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)
> 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
Target Milestone: 3.14.2 → 3.14.3
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.
(In reply to Elio Maldonado from comment #19)
oops, that comment was intended for Bug 835919.
Attachment #747781 - Attachment is obsolete: true
Attachment #748593 - Flags: review?(wtc)
Attachment #748593 - Flags: review?(rrelyea)
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)

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: elio.maldonado.batiz → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
Severity: S3 → N/A
Status: NEW → RESOLVED
Type: defect → enhancement
Closed: 5 months ago
Priority: -- → P5
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: