[review] login: Introduce matches_last_entry to utmp processing

["Carlos O'Donell (Code Review)" <gerrit at gnutoolchain-gerrit dot osci dot io>]

Carlos O'Donell has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/614
......................................................................


Patch Set 1: Code-Review+2

(5 comments)

This refactor and fix for using __utmp_equal-related regression look good to me. It's a great cleanup. I particularly like how the refactoring folds the two while loops.

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

| --- login/utmp_file.c
| +++ login/utmp_file.c
| @@ -127,20 +146,17 @@ __libc_setutent (void)
|        file_fd = __open_nocancel
|  	(file_name, O_RDONLY | O_LARGEFILE | O_CLOEXEC);
|        if (file_fd == -1)
|  	return 0;
|      }
|  
|    __lseek64 (file_fd, 0, SEEK_SET);
|    file_offset = 0;
|  
| -  /* Make sure the entry won't match.  */
| -  last_entry.ut_type = -1;

PS1, Line 137:

OK. We don't need ut_type set to -1 because we detect file_offset == 0
in matches_last_entry, and so call it a failure to match.

| -
|    return 1;
|  }
|  
|  /* Preform initialization if necessary.  */
|  static bool
|  maybe_setutent (void)
|  {
|    return file_fd >= 0 || __libc_setutent ();

 ...

| @@ -185,61 +201,33 @@ __libc_getutent_r (struct utmp *buffer, struct utmp **result)
|  }
|  
|  
|  /* Search for *ID, updating last_entry and file_offset.  Return 0 on
|     success and -1 on failure.  Does not perform locking; for that see
|     internal_getut_r below.  */
|  static int
|  internal_getut_nolock (const struct utmp *id)
|  {
| -  if (id->ut_type == RUN_LVL || id->ut_type == BOOT_TIME
| -      || id->ut_type == OLD_TIME || id->ut_type == NEW_TIME)
| -    {
| -      /* Search for next entry with type RUN_LVL, BOOT_TIME,
| -	 OLD_TIME, or NEW_TIME.  */
| -
| -      while (1)
| -	{
| -	  /* Read the next entry.  */
| -	  if (__read_nocancel (file_fd, &last_entry, sizeof (struct utmp))
| -	      != sizeof (struct utmp))
| -	    {
| -	      __set_errno (ESRCH);
| -	      file_offset = -1l;
| -	      return -1;
| -	    }
| -	  file_offset += sizeof (struct utmp);
| -
| -	  if (id->ut_type == last_entry.ut_type)
| -	    break;
| -	}
| -    }
| -  else
| -    {
| -      /* Search for the next entry with the specified ID and with type
| -	 INIT_PROCESS, LOGIN_PROCESS, USER_PROCESS, or DEAD_PROCESS.  */
| -
| -      while (1)
| -	{
| -	  /* Read the next entry.  */
| -	  if (__read_nocancel (file_fd, &last_entry, sizeof (struct utmp))
| -	      != sizeof (struct utmp))
| -	    {
| -	      __set_errno (ESRCH);
| -	      file_offset = -1l;
| -	      return -1;
| -	    }
| -	  file_offset += sizeof (struct utmp);
| -
| -	  if (__utmp_equal (&last_entry, id))
| -	    break;
| -	}
| +  while (1)
| +    {
| +      /* Read the next entry.  */
| +      if (__read_nocancel (file_fd, &last_entry, sizeof (struct utmp))
| +	  != sizeof (struct utmp))
| +	{
| +	  __set_errno (ESRCH);
| +	  file_offset = -1l;
| +	  return -1;
| +	}
| +      file_offset += sizeof (struct utmp);
| +
| +      if (matches_last_entry (id))
| +	break;
|      }

PS1, Line 236:

OK. The logic of both while loops is effectively the same but with
slightly different exit conditions. The exit conditions of the loops
are refactored and moved into matches_last_entry(), and so it allows
both loops to be merged since they are doing the same work. This looks
good to me.

|  
|    return 0;
|  }
|  
|  /* Search for *ID, updating last_entry and file_offset.  Return 0 on
|     success and -1 on failure.  If the locking operation failed, write
|     true to *LOCK_FAILED.  */
|  static int
|  internal_getut_r (const struct utmp *id, bool *lock_failed)

 ...


-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ib8a85002f7f87ee41590846d16d7e52bdb82f5a5
Gerrit-Change-Number: 614
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Comment-Date: Tue, 12 Nov 2019 16:07:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment