Hi Daniil,

+1

Thanks,
Serguei


On 5/20/19 23:20, David Holmes wrote:
Loosk good.

Thanks,
David

On 21/05/2019 1:25 pm, Daniil Titov wrote:
Please review un updated version of the previous change that also removes unnecessary line

chmod ug+x $REVOKEALL

from test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh

Webrev: http://cr.openjdk.java.net/~dtitov/8214545/webrev.03
Bug: https://bugs.openjdk.java.net/browse/JDK-8214545
     Thanks!
--Daniil

On 5/20/19, 6:02 PM, "serviceability-dev on behalf of Daniil Titov" <serviceability-dev-boun...@openjdk.java.net on behalf of daniil.x.ti...@oracle.com> wrote:

     Please review a new version of the fix that includes the changes David suggested.            > The count-- is obvious as it is the loop counter, but it is far from       >  clear to me that i++ is correct. I don't fully understand the logic
          We need to increment i on line 354:
           353         if (((ACCESS_ALLOWED_ACE *)ace)->Header.AceType != ACCESS_ALLOWED_ACE_TYPE) {
      354             i++;
      355             count--;
      356             continue;
      357         }
          since the code iterates over all ACE entries for a given file and deletes ones that grant non-owner access to the file.  i is the index of the current ACE entry      in the ACL structure. The current ACE entry is retrieved at the beginning of the loop:
          349         if (!GetAce(acl, i, &ace)) {
               and the index is always incremented at the end of the loop unless the current entry is deleted.
          382         if (!deleted) {
      383             str = getSIDString(sid);
      384             if (str != NULL) {
      385                 printf("ALLOW %s (access mask=%x)\n", str, access->Mask);
      386                 free(str);
      387             }
      388
      389             /* onto the next ACE */
      390             i++;
      391         }
      392         count--;
               I also created a new issue to replace revokeall.exe with Java code as Alan suggested : https://bugs.openjdk.java.net/browse/JDK-8224255                Webrev: http://cr.openjdk.java.net/~dtitov/8214545/webrev.02
     Bug: https://bugs.openjdk.java.net/browse/JDK-8214545
          Thanks!
     --Daniil
               On 5/19/19, 5:43 PM, "David Holmes" <david.hol...@oracle.com> wrote:
              Hi Daniil,
                  cc: Boris and Erik J.
                  On 20/05/2019 7:12 am, Daniil Titov wrote:
         > Please review the change that fixes the failure of sun/management/jmxremote/bootstrap JMX tests on Windows platform.  While running, these tests invoke revokeall.exe utility and this utility hangs.
         >
         > The problem here is that invokeall.exe goes into an endless loop  while iterating over Access Control Entries (ACE) for a given file if it encounters at least one ACE with the type different from ACCESS_ALLOWED_ACE_TYPE.
         >
         > The change fixes this problem.  It also removes revokeall.exe binary from the repository and changes the makefile  to get it built instead.
         >
         > Tier1, tier2, tier3, jdk_svc, and sun/management/jmxremote/bootstrap  tests succeeded  in Mach5.
         >
         > Webrev: http://cr.openjdk.java.net/~dtitov/8214545
         > Bug: https://bugs.openjdk.java.net/browse/JDK-8214545
                  I knew this seemed very familiar ... Boris had a fix for this a few          weeks ago under JDK-8220581. Similar but not identical to yours - see          below. Though getting rid of the exe from the repo is a good idea
         (thanks Erik!).
                  A few comments
test/jdk/sun/management/jmxremote/bootstrap/GeneratePropertyPassword.sh
                  Pre-existing:
                  ! REVOKEALL="$TESTNATIVEPATH/revokeall.exe"
                    if [ ! -f "$REVOKEALL" ] ; then
                  I would expect a -x test not -f.
                  ---
                  test/jdk/sun/management/windows/README
                  The first copyright year should be 2004.
                     25 This directory contains the source and the binary version
                  Delete "and the binary version".
                  ---
                  test/jdk/sun/management/windows/exerevokeall.c
                  Pre-existing:
                    31  * file - suitable for NT/2000/XP only.
                  Please delete everything after "file".
                             355             i++;
           356             count--;
                  The count-- is obvious as it is the loop counter, but it is far from          clear to me that i++ is correct. I don't fully understand the logic but          i is only incremented under very specific conditions. If you rewrote the          code to avoid the use of the continue then i would not be modified
         except where it currently is.
                  Thanks,
         David
         -----
                  > Thanks!
         > --Daniil
         >
         >



Reply via email to