[CRIU] [PATCH v2] zdtm: Make file_locks04 test based on /proc/{pid}/fdinfo/{fd}

[Andrew Vagin]

On Thu, Jul 14, 2016 at 01:55:43PM +0300, Pavel Emelyanov wrote:
> Andrey, would you ack/nack this patch?

It's ok for me, but here are a few facts:

* I don't see any reason to fix only this test. Other lock tests has the
  same problems. We use the 'excl' flag to not run these tests
  concurrently.

* If we will fix all tests by this way, we will not be able to test locks
  on kernels where fdinfo doesn't contains locks. We know that in this
  case we can meet the same race in criu and may be it isn't a big
  problem.
> 
> On 06/28/2016 10:08 PM, Kirill Tkhai wrote:
> > /proc/locks is racy with adding/removing locks,
> > so we may lose lock on check.
> > 
> > Use fdinfo's list of locks instead.
> > 
> > v2: Feature 'fdinfo_lock'
> > 
> > Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
> > ---
> >  test/zdtm/static/file_locks04.c    |   31 +++++++++++++++++--------------
> >  test/zdtm/static/file_locks04.desc |    2 +-
> >  2 files changed, 18 insertions(+), 15 deletions(-)
> > 
> > diff --git a/test/zdtm/static/file_locks04.c b/test/zdtm/static/file_locks04.c
> > index 995dd09..28ed497 100644
> > --- a/test/zdtm/static/file_locks04.c
> > +++ b/test/zdtm/static/file_locks04.c
> > @@ -6,6 +6,7 @@
> >  #include <unistd.h>
> >  #include <sys/file.h>
> >  #include <string.h>
> > +#include <limits.h>
> >  #include <sys/wait.h>
> >  
> >  #include "zdtmtst.h"
> > @@ -16,27 +17,27 @@ const char *test_author	= "Pavel Emelyanov <xemul at parallels.com>";
> >  char *filename;
> >  TEST_OPTION(filename, string, "file name", 1);
> >  
> > -static int check_file_locks(int alt_pid, int fd)
> > +static int check_file_locks(pid_t child_pid, int fd, int child_fd)
> >  {
> > +	char		path[PATH_MAX];
> >  	FILE		*fp_locks = NULL;
> >  	char		buf[100], fl_flag[16], fl_type[16], fl_option[16];
> > -	pid_t		pid = getpid();
> >  	int		found = 0, num, fl_owner;
> >  
> > -	fp_locks = fopen("/proc/locks", "r");
> > -	if (!fp_locks)
> > +	sprintf(path, "/proc/%d/fdinfo/%d", child_pid, child_fd);
> > +	fp_locks = fopen(path, "r");
> > +	if (!fp_locks) {
> > +		pr_err("Can't open %s\n", path);
> >  		return -1;
> > -
> > -	test_msg("C: %d/%d\n", pid, alt_pid);
> > +	}
> >  
> >  	while (fgets(buf, sizeof(buf), fp_locks)) {
> > -		test_msg("c: %s", buf);
> > -
> > -		if (strstr(buf, "->"))
> > +		if (strncmp(buf, "lock:\t", 6) != 0)
> >  			continue;
> > +		test_msg("c: %s", buf);
> >  
> >  		num = sscanf(buf,
> > -			"%*d:%s %s %s %d %*02x:%*02x:%*d %*d %*s",
> > +			"%*s %*d:%s %s %s %d %*02x:%*02x:%*d %*d %*s",
> >  			fl_flag, fl_type, fl_option, &fl_owner);
> >  
> >  		if (num < 4) {
> > @@ -44,8 +45,10 @@ static int check_file_locks(int alt_pid, int fd)
> >  			break;
> >  		}
> >  
> > -		if (fl_owner != pid && fl_owner != alt_pid)
> > +		if (fl_owner != child_pid && fl_owner != getpid()) {
> > +			pr_err("Wrong owner\n");
> >  			continue;
> > +		}
> >  
> >  		if (!strcmp(fl_flag, "FLOCK") &&
> >  				!strcmp(fl_type, "ADVISORY") &&
> > @@ -67,11 +70,11 @@ static int check_file_locks(int alt_pid, int fd)
> >  
> >  int main(int argc, char **argv)
> >  {
> > -	int fd, pid;
> > +	int fd, child_fd, pid;
> >  
> >  	test_init(argc, argv);
> >  
> > -	fd = open(filename, O_CREAT | O_RDWR, 0600);
> > +	fd = child_fd = open(filename, O_CREAT | O_RDWR, 0600);
> >  	if (fd < 0) {
> >  		pr_perror("No file");
> >  		return -1;
> > @@ -105,7 +108,7 @@ int main(int argc, char **argv)
> >  	test_daemon();
> >  	test_waitsig();
> >  
> > -	if (check_file_locks(pid, fd))
> > +	if (check_file_locks(pid, fd, child_fd) > 0)
> >  		pass();
> >  	else
> >  		fail("Flock file locks check failed");
> > diff --git a/test/zdtm/static/file_locks04.desc b/test/zdtm/static/file_locks04.desc
> > index 80cd04e..41aad3f 100644
> > --- a/test/zdtm/static/file_locks04.desc
> > +++ b/test/zdtm/static/file_locks04.desc
> > @@ -1 +1 @@
> > -{'flags': 'excl', 'opts': '--file-locks'}
> > +{'flags': 'excl', 'opts': '--file-locks', 'feature': 'fdinfo_lock'}
> > 
> > _______________________________________________
> > CRIU mailing list
> > CRIU at openvz.org
> > https://lists.openvz.org/mailman/listinfo/criu
> > .
> > 
>