delorie.com/archives/browse.cgi   search  
Mail Archives: djgpp-workers/2003/04/02/14:40:21

Date: Wed, 02 Apr 2003 20:43:21 +0100
From: "Richard Dawe" <rich AT phekda DOT freeserve DOT co DOT uk>
Sender: rich AT phekda DOT freeserve DOT co DOT uk
To: djgpp-workers AT delorie DOT com
X-Mailer: Emacs 21.3.50 (via feedmail 8.3.emacs20_6 I) and Blat ver 1.8.6
Subject: fstat, fd_props and inventing inodes, revision 2 [PATCH]
Message-Id: <E190o5E-00018w-00@phekda.freeserve.co.uk>
Reply-To: djgpp-workers AT delorie DOT com

Hello.

Below is revision 2 of the patch to fstat, to make it use the filename
from fd_props, when it can't obtain the inode number from the SFT.
Changes from the previous revision:

* Add the __get_drive_mappings function, to get the list
  of drive mappings in the form (drive number, UNC path).
  Documentation included.

* When we have the filename from fd_props, convert any UNCs
  to drive letters, using the list returned by __get_drive_mappings.

This ensures that fstat will return the same invented inode for the file
whether it was opened via its UNC path (\\machine\share\file)
or through a mapping (x:/file).

Some points:

* It lazily updates the list of shares. The share list is refetched
  every second. Perhaps one second is too short a time period. If so,
  what time period should be used - 5 seconds, 10 seconds?

* The initialisation of drive_mappings is carried out in get_sft_entry.
  Perhaps it would be better initialised somewhere else. Perhaps
  the initialisation code in get_sft_entry should be placed in another
  function - say fstat_init_bss - and fstat_init_bss should be called
  from fstat_assist.

Below is some example output before and after the patch.

OK to commit?

Thanks, bye, Rich =]

Commands to test from bash:

cd src/libc/posix/sys/stat
gcc -DTEST -g -Wall -o fstat fstat.c
net use x: \\\\iolanthe\\zips
./fstat.exe \\iolanthe\\zips\\patch-2.2.24.bz2 x:/patch-2.2.24.bz2

---Before patch---
<olanthe\\zips\\patch-2.2.24.bz2 x:/patch-2.2.24.bz2
handle-0: -1 268435457 20644 1 42 0 1049314660 Wed Apr  2 20:17:40 2003
Everything checks out OK
handle-1: -1 268435457 20644 1 42 0 1049314660 Wed Apr  2 20:17:40 2003
Everything checks out OK
handle-2: -1 268435457 20644 1 42 0 1049314660 Wed Apr  2 20:17:40 2003
Everything checks out OK
handle-3: -1 268435458 20644 1 42 0 1049314660 Wed Apr  2 20:17:40 2003
Everything checks out OK
handle-4: -1 268435459 20644 1 42 0 1049314660 Wed Apr  2 20:17:40 2003
Everything checks out OK
\\iolanthe\zips\patch-2.2.24.bz2 (7): 62 268435460 644 1 42 13801 1046875664 Wed
 Mar  5 14:47:44 2003
                        Times: 1049314292 1046875664
                        Block size: 4096
Failed to get starting cluster number; inode defaults to hashing
(if no other messages were printed, then this is either an empty
file on a local disk drive, or a file on a networked drive, or
you run under some kind of DOS clone)
SFT entry found, but is inconsistent with file size and time stamp
x:/patch-2.2.24.bz2 (8): 23 268435461 644 1 42 13801 1046875664 Wed Mar  5 14:47
:44 2003
                        Times: 1049314292 1046875664
                        Block size: 4096
Failed to get starting cluster number; inode defaults to hashing
(if no other messages were printed, then this is either an empty
file on a local disk drive, or a file on a networked drive, or
you run under some kind of DOS clone)
SFT entry found, but is inconsistent with file size and time stamp

---After patch---
<olanthe\\zips\\patch-2.2.24.bz2 x:/patch-2.2.24.bz2
handle-0: -1 268435457 20644 1 42 0 1049314548 Wed Apr  2 20:15:48 2003
Everything checks out OK
handle-1: -1 268435457 20644 1 42 0 1049314548 Wed Apr  2 20:15:48 2003
Everything checks out OK
handle-2: -1 268435457 20644 1 42 0 1049314548 Wed Apr  2 20:15:48 2003
Everything checks out OK
handle-3: -1 268435458 20644 1 42 0 1049314548 Wed Apr  2 20:15:48 2003
Everything checks out OK
handle-4: -1 268435459 20644 1 42 0 1049314548 Wed Apr  2 20:15:48 2003
Everything checks out OK
\\iolanthe\zips\patch-2.2.24.bz2 (7): 62 268435460 644 1 42 13801 1046875664 Wed
 Mar  5 14:47:44 2003
                        Times: 1049314292 1046875664
                        Block size: 4096
Failed to get starting cluster number; inode defaults to hashing
(if no other messages were printed, then this is either an empty
file on a local disk drive, or a file on a networked drive, or
you run under some kind of DOS clone)
SFT entry found, but is inconsistent with file size and time stamp
x:/patch-2.2.24.bz2 (8): 23 268435460 644 1 42 13801 1046875664 Wed Mar  5 14:47
:44 2003
                        Times: 1049314292 1046875664
                        Block size: 4096
Failed to get starting cluster number; inode defaults to hashing
(if no other messages were printed, then this is either an empty
file on a local disk drive, or a file on a networked drive, or
you run under some kind of DOS clone)
SFT entry found, but is inconsistent with file size and time stamp

Index: include/dir.h
===================================================================
RCS file: /cvs/djgpp/djgpp/include/dir.h,v
retrieving revision 1.4
diff -p -c -3 -r1.4 dir.h
*** include/dir.h	4 Feb 2003 20:23:00 -0000	1.4
--- include/dir.h	2 Apr 2003 19:36:56 -0000
*************** struct ffblklfn {
*** 75,88 ****
  #define DIRECTORY 0x08
  #define DRIVE	  0x10
  
! int	__file_tree_walk(const char *_dir, int (*_fn)(const char *_path, const struct ffblk *_ff));
! int	findfirst(const char *_pathname, struct ffblk *_ffblk, int _attrib);
! int	findnext(struct ffblk *_ffblk);
! void	fnmerge (char *_path, const char *_drive, const char *_dir, const char *_name, const char *_ext);
! int	fnsplit (const char *_path, char *_drive, char *_dir, char *_name, char *_ext);
! int	getdisk(void);
! char *	searchpath(const char *_program);
! int	setdisk(int _drive);
  
  #endif /* !_POSIX_SOURCE */
  #endif /* !__STRICT_ANSI__ */
--- 75,94 ----
  #define DIRECTORY 0x08
  #define DRIVE	  0x10
  
! typedef struct {
!   int  drive;
!   char share[MAXPATH];
! } drive_mapping;
! 
! int		__file_tree_walk(const char *_dir, int (*_fn)(const char *_path, const struct ffblk *_ff));
! int		findfirst(const char *_pathname, struct ffblk *_ffblk, int _attrib);
! int		findnext(struct ffblk *_ffblk);
! void		fnmerge (char *_path, const char *_drive, const char *_dir, const char *_name, const char *_ext);
! int		fnsplit (const char *_path, char *_drive, char *_dir, char *_name, char *_ext);
! int		getdisk(void);
! drive_mapping **__get_drive_mappings (void);
! char *		searchpath(const char *_program);
! int		setdisk(int _drive);
  
  #endif /* !_POSIX_SOURCE */
  #endif /* !__STRICT_ANSI__ */
Index: src/libc/posix/sys/stat/fstat.c
===================================================================
RCS file: /cvs/djgpp/djgpp/src/libc/posix/sys/stat/fstat.c,v
retrieving revision 1.11
diff -p -c -3 -r1.11 fstat.c
*** src/libc/posix/sys/stat/fstat.c	26 Mar 2003 19:53:43 -0000	1.11
--- src/libc/posix/sys/stat/fstat.c	2 Apr 2003 19:37:01 -0000
***************
*** 107,112 ****
--- 107,113 ----
  #include <unistd.h>
  #include <fcntl.h>
  #include <dos.h>
+ #include <dir.h>
  #include <sys/types.h>
  #include <sys/stat.h>
  #include <limits.h>
*************** static int fstat_count = -1;
*** 175,180 ****
--- 176,187 ----
  /* The address of the PSP of the caller.  */
  static unsigned long  psp_addr;
  
+ /* Drive mappings from UNC paths to drive letters. */
+ static drive_mapping **drive_mappings;
+ 
+ /* When were the drive mappings last obtained? */
+ static clock_t drive_mappings_time;
+ 
  /* Initialization routine, called once per program run.
   * Finds DOS version, SFT entry size and addresses of
   * program handle table and first SFT sub-table.
*************** get_sft_entry(int fhandle)
*** 259,264 ****
--- 266,272 ----
      {
        fstat_count = __bss_count;
        dos_major = 0;
+       drive_mappings = NULL;
      }
  
    /* Find the PSP address of the current process.  */
*************** set_fstat_times (int fhandle, struct sta
*** 360,365 ****
--- 368,438 ----
      }
  }
  
+ /* Find a mapping for the UNC path (\\machine\share) in `src', if one
+  * exists.  If one exists, copy the path into `dest' and convert the UNC
+  * to the mapped drive, then return `dest'.  Otherwise, just return `src'.
+  */
+ 
+ static const char *
+ find_mapping_for_unc (const char *src, char *dest)
+ {
+   int is_unc       = 1;
+   drive_mapping *m = NULL;
+   int            i;
+   clock_t        t;
+ 
+   if (!src[0] || (src[0] && !src[1]))
+     is_unc = 0;
+ 
+   if (is_unc && ((src[0] != '\\') || (src[1] != '\\')))
+     is_unc = 0;
+ 
+   if (!is_unc)
+     return src;
+ 
+   /* Update the drive mappings, if: we don't have them; a second has
+    * elapsed; the clock has wrapped.  We update every second as a basic
+    * caching mechanism, but also to keep up with changes in the mappings.
+    *
+    * It may get updated more frequently, if the user calls
+    * __get_drive_mappings, but that should not cause any problems,
+    * because drive_mappings is a pointer to a static buffer.
+    */
+   t = clock();
+ 
+   if (   (drive_mappings == NULL)
+       || ((drive_mappings_time + CLOCKS_PER_SEC) < t)
+       || (t < drive_mappings_time))
+     {
+       drive_mappings = __get_drive_mappings();
+       drive_mappings_time = clock();
+     }
+ 
+   if (drive_mappings != NULL)
+     {
+       for (i = 0; drive_mappings[i] != NULL; i++)
+ 	{
+ 	  if (!strnicmp(drive_mappings[i]->share,
+ 			src,
+ 			strlen(drive_mappings[i]->share)))
+ 	    {
+ 	      m = drive_mappings[i];
+ 	      break;
+ 	    }
+ 	}
+     }
+ 
+   if (m == NULL)
+     return src;
+ 
+   dest[0] = m->drive + 'A';
+   dest[1] = ':';
+   dest[2] = '\0';
+   strcat(dest, src + strlen(m->share));
+ 
+   return dest;
+ }
+ 
  /* fstat_assist() is where all the actual work is done.
   * It uses SFT entry, if available and its contents are verified.
   * Otherwise, it finds all the available info by conventional
*************** fstat_assist(int fhandle, struct stat *s
*** 381,386 ****
--- 454,461 ----
    unsigned short trusted_ftime = 0, trusted_fdate = 0;
    long           trusted_fsize = 0;
    int            is_link = 0;
+   char           fixbuf[PATH_MAX];
+   const char    *filename = "";
  
    if ((dev_info = _get_dev_info(fhandle)) == -1)
      return -1;	/* errno set by _get_dev_info() */
*************** fstat_assist(int fhandle, struct stat *s
*** 434,449 ****
    stat_buf->st_gid = getgid();
    stat_buf->st_nlink = 1;
  
    /* Get the block size for the device associated with `fhandle'. */
  #ifndef  NO_ST_BLKSIZE
!   if (__get_fd_name(fhandle))
      {
!       const char *filename;
!       char fixed_filename[PATH_MAX + 1];
! 
!       filename = __get_fd_name(fhandle);
!       _fixpath(filename, fixed_filename);
!       stat_buf->st_blksize = _get_cached_blksize(fixed_filename);
        if (stat_buf->st_blksize == -1)
  	return -1; /* errno set by _get_cached_blksize() */
      }
--- 509,527 ----
    stat_buf->st_gid = getgid();
    stat_buf->st_nlink = 1;
  
+   /* Get the file name from the file descriptor properties (fd_props),
+    * if possible, and fix it up. */
+   if (__get_fd_name(fhandle))
+     filename = __get_fd_name(fhandle);
+ 
+   if (*filename)
+     filename = find_mapping_for_unc(filename, fixbuf);
+ 
    /* Get the block size for the device associated with `fhandle'. */
  #ifndef  NO_ST_BLKSIZE
!   if (*filename)
      {
!       stat_buf->st_blksize = _get_cached_blksize(filename);
        if (stat_buf->st_blksize == -1)
  	return -1; /* errno set by _get_cached_blksize() */
      }
*************** fstat_assist(int fhandle, struct stat *s
*** 804,816 ****
          }
        else
          {
!           /* Regular file.  The inode will be arbitrary, as we don't have
!            * this file's name.  Sigh...
             */
            if ( (_djstat_flags & _STAT_INODE) == 0 )
              {
                _djstat_fail_bits |= _STFAIL_HASH;
!               stat_buf->st_ino = _invent_inode("", dos_ftime, trusted_fsize);
              }
  
            if (trusted_fsize == 510)
--- 882,897 ----
          }
        else
          {
!           /* Regular file.  We may have obtained this file's name
! 	   * from the file descriptor properties (fd_props).  Otherwise
! 	   * the inode will be arbitrary each time fstat is called.
! 	   * Sigh...
             */
            if ( (_djstat_flags & _STAT_INODE) == 0 )
              {
                _djstat_fail_bits |= _STFAIL_HASH;
!               stat_buf->st_ino
! 		= _invent_inode(filename, dos_ftime, trusted_fsize);
              }
  
            if (trusted_fsize == 510)
*************** fstat(int handle, struct stat *statbuf)
*** 914,920 ****
--- 995,1005 ----
     * use a normal stat call. */
    if (__get_fd_flags(handle) & FILE_DESC_DIRECTORY)
      {
+       char        fixbuf[PATH_MAX];
        const char *filename = __get_fd_name(handle);
+ 
+       if (filename)
+ 	filename = find_mapping_for_unc(filename, fixbuf);
  
        if (filename)
  	return stat(filename, statbuf);
Index: src/libc/posix/sys/stat/fstat.txh
===================================================================
RCS file: /cvs/djgpp/djgpp/src/libc/posix/sys/stat/fstat.txh,v
retrieving revision 1.11
diff -p -c -3 -r1.11 fstat.txh
*** src/libc/posix/sys/stat/fstat.txh	1 Apr 2003 20:47:29 -0000	1.11
--- src/libc/posix/sys/stat/fstat.txh	2 Apr 2003 19:37:10 -0000
*************** files for identity should include compar
*** 85,94 ****
  member.)
  
  3. On all versions of Windows except Windows 3.X, the inode number is
! invented.  As Windows doesn't provide enough information to identify
! files by the handle on which they are open, @code{fstat} always returns
! different inode numbers for any two files open on different handles,
! even if the same file is open twice on two different handles.
  
  4. The WRITE access mode bit is set only for the user (unless the file is
  read-only, hidden or system).  EXECUTE bit is set for directories,  files
--- 85,98 ----
  member.)
  
  3. On all versions of Windows except Windows 3.X, the inode number is
! invented using the file name.  @code{fstat} can probably use the file name
! that was used to open the file, when generating the inode.  This is done
! such that the same inode will be generated irrespective of the actual path
! used to open the file (e.g.: @samp{foo.txt}, @samp{./foo.txt},
! @samp{../somedir/foo.txt}).  If file names cannot be used, @code{fstat}
! always returns different inode numbers for any two files open
! on different handles, even if the same file is open twice
! on two different handles.
  
  4. The WRITE access mode bit is set only for the user (unless the file is
  read-only, hidden or system).  EXECUTE bit is set for directories,  files
Index: src/docs/kb/wc204.txi
===================================================================
RCS file: /cvs/djgpp/djgpp/src/docs/kb/wc204.txi,v
retrieving revision 1.151
diff -p -c -3 -r1.151 wc204.txi
*** src/docs/kb/wc204.txi	26 Mar 2003 19:54:46 -0000	1.151
--- src/docs/kb/wc204.txi	2 Apr 2003 19:37:15 -0000
*************** to the POSIX functions @code{open}, @cod
*** 944,946 ****
--- 944,953 ----
  @code{fsync} and @code{fdopen} and the ANSI functions @code{fopen},
  @code{freopen}, @code{fclose}, @code{ftell}, @code{fseek}
  and @code{rewind}, to make them aware of file descriptors for directories.
+ 
+ @findex fstat AT r{, and inodes}
+ @code{fstat} will now use the file name used to open the file,
+ when inventing inodes.  This is done so that the same inode is generated
+ irrespective of the actual file path used to open the file.  This also
+ fixes the problem where multiple calls to fstat
+ on the same file descriptor would give different inodes.
*** /dev/null	Wed Apr  2 20:41:17 2003
--- src/libc/dos/dir/getshare.c	Wed Apr  2 13:31:10 2003
***************
*** 0 ****
--- 1,179 ----
+ /* Copyright (C) 2003 DJ Delorie, see COPYING.DJ for details */
+ #include <libc/stubs.h>
+ #include <limits.h>
+ #include <stdlib.h>
+ #include <string.h>
+ #include <errno.h>
+ #include <dir.h>
+ #include <dpmi.h>
+ #include <go32.h>
+ #include <sys/movedata.h>
+ #include <libc/dosio.h>
+ #include <libc/bss.h>
+ 
+ /* LANMan's use_info_1 structure */
+ typedef struct {
+   char  local_name[9];
+   char  padding;
+   short remote_name_offset;
+   short remote_name_segment;
+   short password_offset;
+   short password_segment;
+   short status;
+   short type;
+   short ignored;
+   short ignored2;
+ } share_info __attribute__((packed));
+ 
+ #define SHARE_STATUS_OK           0
+ #define SHARE_STATUS_DISCONNECTED 2
+ 
+ #define SHARE_TYPE_WILDCARD -1
+ #define SHARE_TYPE_DISK      0
+ #define SHARE_TYPE_PRINT     1
+ #define SHARE_TYPE_COM       2
+ #define SHARE_TYPE_IPC       3
+ 
+ #ifndef __tb_size
+ #define __tb_size _go32_info_block.size_of_transfer_buffer
+ #endif
+ 
+ /* Allow for 32 drives plus NULL. */
+ #define N_DRIVES 32
+ static drive_mapping *drive_mappings[N_DRIVES + 1];
+ static share_info    *share_infos;
+ static size_t         n_share_infos;
+ static size_t         n_share_infos_max;
+ 
+ static void
+ shares_init (void)
+ {
+   static int          shares_bss_count      = -1;
+   static const size_t default_n_share_infos = N_DRIVES;
+   size_t              i;
+ 
+   if (shares_bss_count != __bss_count) {
+     shares_bss_count = __bss_count;
+ 
+     n_share_infos_max = __tb_size / sizeof(share_info);
+ 
+     /* Ensure we don't overflow the transfer buffer. We shouldn't
+      * since it's big enough to store a lot of share info. */
+     n_share_infos = default_n_share_infos;
+     if (n_share_infos > n_share_infos_max)
+       n_share_infos = n_share_infos_max;
+ 
+     for (i = 0; i < sizeof(drive_mappings) / sizeof(drive_mappings[0]); i++) {
+       drive_mappings[i] = NULL;
+     }
+ 
+     share_infos = malloc(n_share_infos * sizeof(*share_infos));
+     if (share_infos == NULL)
+       n_share_infos = 0;
+   }
+ }
+ 
+ static int
+ get_shares_internal (share_info *shares, const size_t n_shares)
+ {
+   __dpmi_regs r;
+ 
+   r.x.ax = 0x5f46; /* LANMan: NetUseEnum */
+   r.x.bx = 0x0001;
+   r.x.cx = sizeof(*shares) * n_shares;
+   r.x.es = __tb >> 4;
+   r.x.di = __tb & 0xf;
+ 
+   __dpmi_int(0x21, &r);
+ 
+   if(r.x.flags & 1) {
+     errno = __doserr_to_errno(r.x.ax);
+     return(-1);
+   }
+ 
+   dosmemget(__tb, r.x.cx * sizeof(*shares), shares);
+ 
+   return(r.x.cx);
+ }
+ 
+ drive_mapping **
+ __get_drive_mappings (void)
+ {
+   drive_mapping **p = NULL; /* Fail by default. */
+   int             drive;
+   char            path[MAXPATH];
+   int             ret, addr, pos, i;
+ 
+   shares_init();
+ 
+   ret = get_shares_internal(share_infos, n_share_infos);
+ 
+   /* Error or no shares available. */
+   if ((ret < 0) || !ret)
+     return(p);
+ 
+   for (pos = i = 0; (i < ret) && (pos < N_DRIVES); i++) {
+     drive = share_infos[i].local_name[0];
+ 
+     /* Skip non-disks & disks without a mapping. */
+     if ((share_infos[i].type != SHARE_TYPE_DISK) || (drive == '\0'))
+       continue;
+ 
+     /* Check the drive is in range. */
+     if ((drive >= 'a') && (drive <= 'z'))
+       drive -= 'a';
+     else
+       drive -= 'A';
+ 
+     if (drive >= N_DRIVES)
+       continue;
+ 
+     /* Get the UNC - the remote name. */
+     addr  = share_infos[i].remote_name_segment << 4;
+     addr += share_infos[i].remote_name_offset;
+     dosmemget(addr, sizeof(path), path);
+     path[sizeof(path) - 1] = '\0';
+ 
+     if (drive_mappings[pos] == NULL)
+       drive_mappings[pos] = malloc(sizeof(drive_mapping));
+ 
+     if (drive_mappings[pos] != NULL) {
+       drive_mappings[pos]->drive = drive;
+       strcpy(drive_mappings[pos]->share, path);
+       pos++;
+     }
+   }
+ 
+   /* Clean out stale mappings. */
+   for (i = pos; i <= N_DRIVES; i++) {
+     if (drive_mappings[pos] != NULL) {
+       free(drive_mappings[pos]);
+       drive_mappings[pos] = NULL;
+     }
+   }
+ 
+   p = drive_mappings;
+ 
+   return(p);
+ }
+ 
+ #ifdef TEST
+ 
+ #include <stdio.h>
+ 
+ int
+ main (void)
+ {
+   drive_mapping **p = __get_drive_mappings();
+   int i;
+ 
+   if (p != NULL) {
+     for (i = 0; p[i] != NULL; i++) {
+       printf("%c -> %s\n", p[i]->drive + 'A', p[i]->share);
+     }
+   }
+ 
+   return(EXIT_SUCCESS);
+ }
+ 
+ #endif /* TEST */
*** /dev/null	Wed Apr  2 20:41:17 2003
--- src/libc/dos/dir/getshare.txh	Wed Apr  2 13:31:52 2003
***************
*** 0 ****
--- 1,54 ----
+ @node __get_drive_mappings, dos
+ @findex __get_drive_mappings
+ @tindex drive_mapping
+ @cindex Universal Naming Convention
+ @cindex UNC
+ 
+ @subheading Syntax
+ 
+ @example
+ #include <dir.h>
+ 
+ drive_mapping **__get_drive_mappings (void);
+ @end example
+ 
+ @subheading Description
+ 
+ This function returns a list of the shares currently mapped
+ to drive letters.  A share is a @dfn{Universal Naming Convention} (@dfn{UNC})
+ path of the form @file{\\machine\share}.
+ 
+ @code{drive_mapping} is defined as follows:
+ 
+ @example
+ typedef struct @{
+   int  drive;
+   char share[MAXPATH];
+ @} drive_mapping;
+ @end example
+ 
+ @code{drive} is the drive number, where @file{A:} is 0, @file{B:} is 1, etc.
+ @code{share} is the UNC path.
+ 
+ @subheading Return Value
+ 
+ NULL, if there are no shares; otherwise a NULL-terminated list
+ of pointers to @code{drive_mapping}s.  The returned list is to static
+ buffers and should not be modified by caller.
+ 
+ @subheading Portability
+ 
+ @portability !ansi, !posix
+ 
+ @subheading Example
+ 
+ @example
+ drive_mapping **p = __get_drive_mappings();
+ int i;
+ 
+ if (p != NULL) @{
+   for (i = 0; p[i] != NULL; i++) @{
+     printf("%c -> %s\n", p[i]->drive + 'A', p[i]->share);
+   @}
+ @}
+ @end example

- Raw text -


  webmaster     delorie software   privacy  
  Copyright © 2019   by DJ Delorie     Updated Jul 2019