129201 - ckfw needs an accessor for module initialization arguments

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

Comment 1

[Julien Pierre]

Attachment: Add new accessor function to retrieve module initialization parameters

Comment 2

[Julien Pierre]

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.

Comment 3

[Wan-Teh Chang]

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.

Comment 4

[Julien Pierre]

Attachment: updated patch with wtc's suggestions

Comment 5

[Wan-Teh Chang]

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.

Comment 6

[Julien Pierre]

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

Comment 7

[Julien Pierre]

checked in fix

Comment 8

[Julien Pierre]

verified