ckfw needs an accessor for module initialization arguments

VERIFIED FIXED in 3.4

Status

P1
normal
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: julien.pierre, Assigned: wtc)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

17 years ago
 
(Reporter)

Comment 1

17 years ago
Created attachment 72727 [details] [diff] [review]
Add new accessor function to retrieve module initialization parameters
(Reporter)

Comment 2

17 years ago
In a PKCS#11 module sitting on top of ckfw, it is not possible to retrieve the
module parameters passed from C_Initialize when overriding the create instance
function because the NSSCKFWInstance structure is opaque . The fix is to add an
accessor function, which I have provided.
Priority: -- → P1
Target Milestone: --- → 3.4
(Assignee)

Comment 3

17 years ago
Comment on attachment 72727 [details] [diff] [review]
Add new accessor function to retrieve module initialization parameters

This patch is basically fine.  I only find one
minor error (casting NULL to the wrong type).
But there are some stylistic changes I'd like
to suggest.

Although NSS has some coding standards, they
have not been strictly enforced.  When modifying
a file, it is important to follow the existing
styles in the file.

My comments on your patch are inlined.

1. mozilla/security/nss/lib/ckfw/instance.c

>@@ -502,7 +504,7 @@
> }
> 
> /*
>- * nssCKFWInstance_GetConfigurationData
>+ * 
>  *
>  */
> NSS_IMPLEMENT NSSUTF8 *

This line is accidentally deleted.

>+CK_C_INITIALIZE_ARGS_PTR
>+nssCKFWInstance_GetInitArgs
>+(
>+  NSSCKFWInstance *fwInstance
>+)
>+{
>+    if (fwInstance)
>+    {
>+        return fwInstance->pInitArgs;
>+    }
>+    else
>+    {
>+        return NULL;
>+    }

This function looks very different from the
functions next to it.  To emulate the original
styles (of Fred's), you can copy the
nssCKFWInstance_GetConfigurationData function,
including the comment in front of it, and modify
it.

>+/*
>+ * NSSCKFWInstance_GetInitArgs
>+ *
>+ */
>+NSS_IMPLEMENT CK_C_INITIALIZE_ARGS_PTR
>+NSSCKFWInstance_GetInitArgs
>+(
>+  NSSCKFWInstance *fwInstance
>+)
>+{
>+#ifdef DEBUG
>+  if( CKR_OK != nssCKFWInstance_verifyPointer(fwInstance) ) {
>+    return (NSSUTF8 *)NULL;
>+  }
>+#endif /* DEBUG */
>+
>+  return nssCKFWInstance_GetInitArgs(fwInstance);
>+}

The (NSSUTF8 *)NULL cast should be changed to
(CK_C_INITIALIZE_ARGS_PTR)NULL.

2. mozilla/security/nss/lib/ckfw/nssckfw.h

>+NSS_EXTERN CK_C_INITIALIZE_ARGS_PTR
>+NSSCKFWInstance_GetInitArgs
>+(
>+  NSSCKFWInstance *fwInstance
>+);

Similar suggestion here.  Add a header comment in
front of this function declaration to emulate the
style of the existing code.
Attachment #72727 - Flags: needs-work+
(Reporter)

Comment 4

17 years ago
Created attachment 72842 [details] [diff] [review]
updated patch with wtc's suggestions
(Reporter)

Updated

17 years ago
Attachment #72727 - Attachment is obsolete: true
(Assignee)

Comment 5

17 years ago
Comment on attachment 72842 [details] [diff] [review]
updated patch with wtc's suggestions

Julien,

You just need to fix one error in this patch and you
can check it in.

In mozilla/security/nss/lib/ckfw/instance.c

> /*
>+ * nssCKFWInstance_InitArgs
>+ *
>+ */

This comment should say "nssCKFWInstance_GetInitArgs". :-)

Thanks.
Attachment #72842 - Flags: review+
(Reporter)

Comment 6

17 years ago
I think I need a vacation, more sleep, or both. I should have spotted that one.

Checking in instance.c;
/cvsroot/mozilla/security/nss/lib/ckfw/instance.c,v  <--  instance.c
new revision: 1.5; previous revision: 1.4
done
Checking in nssckfw.h;
/cvsroot/mozilla/security/nss/lib/ckfw/nssckfw.h,v  <--  nssckfw.h
new revision: 1.2; previous revision: 1.1
done
(Reporter)

Comment 7

17 years ago
checked in fix
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Reporter)

Comment 8

17 years ago
verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.