| Message ID | gerrit.1573559410000.Ib8a85002f7f87ee41590846d16d7e52bdb82f5a5@gnutoolchain-gerrit.osci.io |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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
diff --git a/login/utmp_file.c b/login/utmp_file.c index 917c4c5..c41d17b 100644 --- a/login/utmp_file.c +++ b/login/utmp_file.c @@ -43,6 +43,25 @@ /* Cache for the last read entry. */ static struct utmp last_entry; +/* Returns true if *ENTRY matches last_entry, based on + data->ut_type. */ +static bool +matches_last_entry (const struct utmp *data) +{ + if (file_offset <= 0) + /* Nothing has been read. last_entry is stale and cannot match. */ + return false; + + if (data->ut_type == RUN_LVL + || data->ut_type == BOOT_TIME + || data->ut_type == OLD_TIME + || data->ut_type == NEW_TIME) + /* For some entry types, only a type match is required. */ + return data->ut_type == last_entry.ut_type; + else + /* For the process-related entries, a full match is needed. */ + return __utmp_equal (&last_entry, data); +} /* Locking timeout. */ #ifndef TIMEOUT @@ -133,9 +152,6 @@ __lseek64 (file_fd, 0, SEEK_SET); file_offset = 0; - /* Make sure the entry won't match. */ - last_entry.ut_type = -1; - return 1; } @@ -191,48 +207,20 @@ 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) + while (1) { - /* 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)) { - /* 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; + __set_errno (ESRCH); + file_offset = -1l; + return -1; } - } - else - { - /* Search for the next entry with the specified ID and with type - INIT_PROCESS, LOGIN_PROCESS, USER_PROCESS, or DEAD_PROCESS. */ + file_offset += sizeof (struct utmp); - 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; - } + if (matches_last_entry (id)) + break; } return 0; @@ -365,13 +353,7 @@ /* Find the correct place to insert the data. */ bool found = false; - if (file_offset > 0 - && ((last_entry.ut_type == data->ut_type - && (last_entry.ut_type == RUN_LVL - || last_entry.ut_type == BOOT_TIME - || last_entry.ut_type == OLD_TIME - || last_entry.ut_type == NEW_TIME)) - || __utmp_equal (&last_entry, data))) + if (matches_last_entry (data)) { if (__lseek64 (file_fd, file_offset, SEEK_SET) < 0) { @@ -389,7 +371,7 @@ found = false; } else - found = __utmp_equal (&last_entry, data); + found = matches_last_entry (data); } if (!found)
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/614 ...................................................................... login: Introduce matches_last_entry to utmp processing This simplifies internal_getut_nolock and fixes a regression, introduced in commit be6b16d975683e6cca57852cd4cfe715b2a9d8b1 ("login: Acquire write lock early in pututline [BZ #24882]") in pututxline because __utmp_equal can only compare process-related utmp entries. Fixes: be6b16d975683e6cca57852cd4cfe715b2a9d8b1 Change-Id: Ib8a85002f7f87ee41590846d16d7e52bdb82f5a5 --- M login/utmp_file.c 1 file changed, 31 insertions(+), 49 deletions(-) -- 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-MessageType: newchange