aboutsummaryrefslogtreecommitdiff
path: root/libio
AgeCommit message (Collapse)AuthorFilesLines
2024-09-05Fix freopen handling of ,ccs= (bug 23675)Joseph Myers2-6/+3
As reported in bug 23675 and shown up in the recently added tests of different cases of freopen (relevant part of the test currently conditioned under #if 0 to avoid a failure resulting from this bug), freopen wrongly forces the stream to unoriented even when a mode with ,ccs= is specified, though such a mode is supposed to result in a wide-oriented stream. Move the clearing of _mode to before the actual reopening occurs, so that the main fopen implementation can leave a wide-oriented stream in the ,ccs= case. Tested for x86_64.
2024-09-05Test fclose on an unopened file.Aaron Merey2-0/+41
Add new file libio/tst-fclosed-unopened.c that tests whether fclose on an unopened file returns EOF. Calling fclose on unopened files normally causes a use-after-free bug, however the standard streams are an exception since they are not deallocated by fclose. fclose returning EOF for unopened files is not part of the external contract but there are dependancies on this behaviour. For example, gnulib's close_stdout in lib/closeout.c. Tested for x86_64. Signed-off-by: Aaron Merey <amerey@redhat.com>
2024-09-05Fix memory leak on freopen error return (bug 32140)Joseph Myers2-0/+4
As reported in bug 32140, freopen leaks the FILE object when it returns NULL: there is no valid use of the FILE * pointer (including passing to freopen again or to fclose) after such an error return, so the underlying object should be freed. Add code to free it. Note 1: while I think it's clear from the relevant standards that the object should be freed and the FILE * can't be used after the call in this case (the stream is closed, which ends the lifetime of the FILE), it's entirely possible that some existing code does in fact try to use the existing FILE * in some way and could be broken by this change. (Though the most common case for freopen may be stdin / stdout / stderr, which _IO_deallocate_file explicitly checks for and does not deallocate.) Note 2: the deallocation is only done in the _IO_IS_FILEBUF case. Other kinds of streams bypass all the freopen logic handling closing the file, meaning a call to _IO_deallocate_file would neither be safe (the FILE might still be linked into the list of all open FILEs) nor sufficient (other internal memory allocations associated with the file would not have been freed). I think the validity of freopen for any other kind of stream will need clarifying with the Austin Group, but if it is valid in any such case (where "valid" means "not undefined behavior so required to close the stream" rather than "required to successfully associate the stream with the new file in cases where fopen would work"), more significant changes would be needed to ensure the stream gets fully closed. Tested for x86_64.
2024-09-05Clear flags2 flags set from mode in freopen (bug 32134)Joseph Myers2-0/+9
As reported in bug 32134, freopen does not clear the flags set in fp->_flags2 by the "e", "m" or "c" mode characters. Clear these so that they can be set or not as appropriate from the mode string passed to freopen. The relevant test for "e" in tst-freopen2-main.c is enabled accordingly; "c" is expected to be covered in a separately written test (and while tst-freopen2-main.c does include transitions to and from "m", that's not really a semantic flag intended to result in behaving in an observably different way). Tested for x86_64.
2024-09-04libio: Attempt wide backup free only for non-legacy codeSiddhesh Poyarekar1-1/+1
_wide_data and _mode are not available in legacy code, so do not attempt to free the wide backup buffer in legacy code. Resolves: BZ #32137 and BZ #27821 Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> Reviewed-by: Florian Weimer <fweimer@redhat.com>
2024-08-15ungetc: Fix backup buffer leak on program exit [BZ #27821]Siddhesh Poyarekar2-2/+8
If a file descriptor is left unclosed and is cleaned up by _IO_cleanup on exit, its backup buffer remains unfreed, registering as a leak in valgrind. This is not strictly an issue since (1) the program should ideally be closing the stream once it's not in use and (2) the program is about to exit anyway, so keeping the backup buffer around a wee bit longer isn't a real problem. Free it anyway to keep valgrind happy when the streams in question are the standard ones, i.e. stdout, stdin or stderr. Also, the _IO_have_backup macro checks for _IO_save_base, which is a roundabout way to check for a backup buffer instead of directly looking for _IO_backup_base. The roundabout check breaks when the main get area has not been used and user pushes a char into the backup buffer with ungetc. Fix this to use the _IO_backup_base directly. Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
2024-08-15ungetc: Fix uninitialized read when putting into unused streams [BZ #27821]Siddhesh Poyarekar1-1/+1
When ungetc is called on an unused stream, the backup buffer is allocated without the main get area being present. This results in every subsequent ungetc (as the stream remains in the backup area) checking uninitialized memory in the backup buffer when trying to put a character back into the stream. Avoid comparing the input character with buffer contents when in backup to avoid this uninitialized read. The uninitialized read is harmless in this context since the location is promptly overwritten with the input character, thus fulfilling ungetc functionality. Also adjust wording in the manual to drop the paragraph that says glibc cannot do multiple ungetc back to back since with this change, ungetc can actually do this. Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
2024-08-14libio/tst-getdelim: Add new test covering NUL as a delimiterFrédéric Bérat1-1/+21
Add a new test to getdelim to verify that '\0' can be set as a delimiter. Reviewed-by: Florian Weimer <fweimer@redhat.com>
2024-08-05Fix name space violation in fortify wrappers (bug 32052)Andreas Schwab1-20/+20
Rename the identifier sz to __sz everywhere. Fixes: a643f60c53 ("Make sure that the fortified function conditionals are constant")
2024-07-09libio: handle opening a file when all files are closed (bug 31963)Andreas Schwab3-1/+37
_IO_list_all becomes NULL when all files (including standard files) are closed.
2024-07-01Fix conditionals on mtrace-based tests (bug 31892)Carlos O'Donell1-11/+31
The conditionals for several mtrace-based tests in catgets, elf, libio, malloc, misc, nptl, posix, and stdio-common were incorrect leading to test failures when bootstrapping glibc without perl. The correct conditional for mtrace-based tests requires three checks: first checking for run-built-tests, then build-shared, and lastly that PERL is not equal to "no" (missing perl). Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
2024-06-11<stdio.h>: Acknowledge that getdelim/getline are in POSIXPhilip Kaludercic1-12/+2
These comments were written in 2003 (added in 2c008571c3a), predating the addition of getdelim(3)/getline(3) in POSIX.1-2008. Reviewed-by: Sam James <sam@gentoo.org> Reviewed-by: Florian Weimer <fweimer@redhat.com>
2024-06-04libio: Test for fdopen memory leak without SEEK_END support (bug 31840)Florian Weimer2-2/+64
The bug report used /dev/mem, but /proc/self/mem works as well (if available).
2024-06-04Remove memory leak in fdopen (bug 31840)Andreas Schwab1-1/+5
Deallocate the memory for the FILE structure when seeking to the end fails in append mode. Fixes: ea33158c96 ("Fix offset caching for streams and use it for ftell (BZ #16680)")
2024-05-21Change _IO_stderr_/_IO_stdin_/_IO_stdout to compat symbols [BZ #31766]H.J. Lu1-0/+4
Since Glibc never provides symbol binary compatibility for relocatable files, fix BZ #31766 by changing _IO_stderr_/_IO_stdin_/_IO_stdout to compat symbols. Signed-off-by: H.J. Lu <hjl.tools@gmail.com> Reviewed-by: Sunil K Pandey <skpgkp2@gmail.com>
2024-05-17Use a doubly-linked list for _IO_list_all (bug 27777)Alexandre Ferrieux3-2/+43
This patch fixes BZ #27777 "fclose does a linear search, takes ages when many FILE* are opened". Simply put, the master list of opened (FILE*), namely _IO_list_all, is a singly-linked list. As a consequence, the removal of a single element is in O(N), which cripples the performance of fclose(). The patch switches to a doubly-linked list, yielding O(1) removal. The one padding field in struct _IO_FILE, __pad5, is renamed to _prevchain for a doubly-linked list. Since fields in struct _IO_FILE after the _lock field are internal to glibc and opaque to applications. We can change them as long as the size of struct _IO_FILE is unchanged, which is checked as the part of glibc ABI with sizes of _IO_2_1_stdin_, _IO_2_1_stdout_ and _IO_2_1_stderr_. NB: When _IO_vtable_offset (fp) == 0, copy relocation will cover the whole struct _IO_FILE. Otherwise, only fields up to the _lock field will be copied to applications at run-time. It is used to check if the _prevchain field can be safely accessed. After opening 2 million (FILE*), the fclose() of 100 of them takes quite a few seconds without the patch, and under 2 seconds with it on a loaded machine. No test is added since there are no functional changes. Co-Authored-By: H.J. Lu <hjl.tools@gmail.com> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@orange.com> Signed-off-by: H.J. Lu <hjl.tools@gmail.com> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
2024-05-13libio/bug-wsetpos: Make the error message match the causing functionMaciej W. Rozycki1-1/+1
This test case calls `fopen': FILE *fp = fopen (temp_file, "r"); however if that fails it reports `fdopen' being the origin of the error. Adjust the message to say `fopen' then.
2024-05-06Add crt1-2.0.o for glibc 2.0 compatibility testsH.J. Lu2-0/+58
Starting from glibc 2.1, crt1.o contains _IO_stdin_used which is checked by _IO_check_libio to provide binary compatibility for glibc 2.0. Add crt1-2.0.o for tests against glibc 2.0. Define tests-2.0 for glibc 2.0 compatibility tests. Add and update glibc 2.0 compatibility tests for stderr, matherr and pthread_kill. Reviewed-by: Carlos O'Donell <carlos@redhat.com>
2024-04-30libio: Sort test variables in MakefileH.J. Lu1-20/+84
Sort test variables in libio/Makefile using scripts/sort-makefile-lines.py. Reviewed-by: Sunil K Pandey <skpgkp2@gmail.com>
2024-04-22Revert "Allow glibc to be compiled without EXEC_PAGESIZE"Samuel Thibault1-1/+1
This reverts commit 49aa652db810ebdca3a662ebd5b0468bd08ec688. This is still being discussed.
2024-04-10posix: Sync tempname with gnulibAdhemerval Zanella1-0/+1
The gnulib version contains an important change (9ce573cde), which fixes some problems with multithreading, entropy loss, and ASLR leak nfo. It also fixes an issue where getrandom is not being used on some new files generation (only for __GT_NOCREATE on first try). The 044bf893ac removed __path_search, which is now moved to another gnulib shared files (stdio-common/tmpdir.{c,h}). Tthis patch also fixes direxists to use __stat64_time64 instead of __xstat64, and move the include of pathmax.h for !_LIBC (since it is not used by glibc). The license is also changed from GPL 3.0 to 2.1, with permission from the authors (Bruno Haible and Paul Eggert). The sync also removed the clock fallback, since clock_gettime with CLOCK_REALTIME is expected to always succeed. It syncs with gnulib commit 323834962817af7b115187e8c9a833437f8d20ec. Checked on x86_64-linux-gnu. Co-authored-by: Bruno Haible <bruno@clisp.org> Co-authored-by: Paul Eggert <eggert@cs.ucla.edu> Reviewed-by: Bruno Haible <bruno@clisp.org>
2024-03-23Allow glibc to be compiled without EXEC_PAGESIZESergey Bugaev1-1/+1
We would like to avoid statically defining any specific page size on aarch64-gnu, and instead make sure that everything uses the dynamic page size, available via vm_page_size and GLRO(dl_pagesize). There are currently a few places in glibc that require EXEC_PAGESIZE to be defined. Per Roland's suggestion [0], drop the static GLRO(dl_pagesize) initializers (for now, only if EXEC_PAGESIZE is not defined), and don't require EXEC_PAGESIZE definition for libio to enable mmap usage. [0]: https://mail.gnu.org/archive/html/bug-hurd/2011-10/msg00035.html Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> Message-ID: <20240323173301.151066-4-bugaevc@gmail.com>
2024-02-27libio: Improve fortify with clangAdhemerval Zanella1-20/+153
It improve fortify checks for sprintf, vsprintf, vsnsprintf, fprintf, dprintf, asprintf, __asprintf, obstack_printf, gets, fgets, fgets_unlocked, fread, and fread_unlocked. The runtime checks have similar support coverage as with GCC. For function with variadic argument (sprintf, snprintf, fprintf, printf, dprintf, asprintf, __asprintf, obstack_printf) the fortify wrapper calls the va_arg version since clang does not support __va_arg_pack. Checked on aarch64, armhf, x86_64, and i686. Reviewed-by: Carlos O'Donell <carlos@redhat.com> Tested-by: Carlos O'Donell <carlos@redhat.com>
2024-02-01Refer to C23 in place of C2X in glibcJoseph Myers2-7/+7
WG14 decided to use the name C23 as the informal name of the next revision of the C standard (notwithstanding the publication date in 2024). Update references to C2X in glibc to use the C23 name. This is intended to update everything *except* where it involves renaming files (the changes involving renaming tests are intended to be done separately). In the case of the _ISOC2X_SOURCE feature test macro - the only user-visible interface involved - support for that macro is kept for backwards compatibility, while adding _ISOC23_SOURCE. Tested for x86_64.
2024-01-02libio: Check remaining buffer size in _IO_wdo_write (bug 31183)Florian Weimer1-1/+1
The multibyte character needs to fit into the remaining buffer space, not the already-written buffer space. Without the fix, we were never moving the write pointer from the start of the buffer, always using the single-character fallback buffer. Fixes commit 04b76b5aa8b2d1d19066e42dd1 ("Don't error out writing a multibyte character to an unbuffered stream (bug 17522)").
2024-01-01Update copyright dates with scripts/update-copyrightsPaul Eggert179-179/+179
2023-09-26libio: Add nonnull attribute for most FILE * arguments in stdio.hXi Ruoyao3-78/+99
During the review of a GCC analyzer test case, we found most stdio functions accepting a FILE * argument expect it to be nonnull and just segfault when the argument is NULL. Add nonnull attribute for them. fflush and fflush_unlocked are well defined when __stream is NULL so they are not touched. For fputs, fgets, fread, fwrite, fprintf, vfprintf, and their unlocked version, if __stream is empty but there is nothing to read or write, they did not segfault. But the standard disallow __stream to be empty here, so nonnull attribute is also added for them. Note that this may blow up some old code already subtly broken. Also add __nonnull for _chk variants and __fortify_function versions for them. Signed-off-by: Xi Ruoyao <xry111@xry111.site> Reviewed-by: Alejandro Colomar <alx@kernel.org> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
2023-09-08libio: Fix oversized __io_vtablesAdam Jackson1-1/+4
IO_VTABLES_LEN is the size of the struct array in bytes, not the number of __IO_jump_t's in the array. Drops just under 384kb from .rodata on LP64 machines. Fixes: 3020f72618e ("libio: Remove the usage of __libc_IO_vtables") Signed-off-by: Adam Jackson <ajax@redhat.com> Reviewed-by: Florian Weimer <fweimer@redhat.com> Tested-by: Florian Weimer <fweimer@redhat.com>
2023-07-20hurd: Fix tst-openlocSamuel Thibault1-1/+2
On GNU/Hurd, O_RDWR actually is O_WRONLY|O_RDONLY, so checking through bitness really is wrong. O_ACCMODE is there for this. Fixes: 5324d258427f ("fileops: Don't process ,ccs= as individual mode flags (BZ#18906)")
2023-07-05fileops: Don't process ,ccs= as individual mode flags (BZ#18906)Joe Simmons-Talbott2-1/+34
In processing the first 7 individual characters of the mode for fopen if ,ccs= is used those characters will be processed as well. Stop processing individual mode flags once a comma is encountered. This has the effect of requiring ,ccs= to be the last mode flag in the mode string. Add a testcase to check that the ,ccs= mode flag is not processed as individual mode flags. Reviewed-by: DJ Delorie <dj@redhat.com>
2023-07-05libio/bits/stdio2-decl.h: Avoid PLT entries with _FORTIFY_SOURCEFrédéric Bérat1-1/+1
The change is meant to avoid unwanted PLT entry for the fgets_unlocked routine when _FORTIFY_SOURCE is set. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
2023-07-05libio/bits/stdio2.h: Clearly separate declaration from definitionsFrédéric Bérat2-48/+49
Move declarations from libio/bits/stdio.h to existing libio/bits/stdio2-decl.h. This will enable future use of __REDIRECT_FORTIFY in place of some __REDIRECT. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
2023-07-05Exclude routines from fortificationFrédéric Bérat1-2/+21
Since the _FORTIFY_SOURCE feature uses some routines of Glibc, they need to be excluded from the fortification. On top of that: - some tests explicitly verify that some level of fortification works appropriately, we therefore shouldn't modify the level set for them. - some objects need to be build with optimization disabled, which prevents _FORTIFY_SOURCE to be used for them. Assembler files that implement architecture specific versions of the fortified routines were not excluded from _FORTIFY_SOURCE as there is no C header included that would impact their behavior. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
2023-07-03Always do locking when accessing streams (bug 15142, bug 14697)Andreas Schwab2-34/+10
Now that abort no longer calls fflush there is no reason to avoid locking the stdio streams anywhere. This fixes a conformance issue and potential heap corruption during exit.
2023-06-19tests: replace system by xsystemFrédéric Bérat1-2/+5
With fortification enabled, system calls return result needs to be checked, has it gets the __wur macro enabled. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
2023-06-13tests: replace fread by xfreadFrederic Berat1-3/+4
With fortification enabled, fread calls return result needs to be checked, has it gets the __wur macro enabled. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
2023-06-02Fix all the remaining misspellings -- BZ 25337Paul Pluzhnikov6-6/+6
2023-06-01tests: replace write by xwriteFrédéric Bérat5-5/+15
Using write without cheks leads to warn unused result when __wur is enabled. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
2023-05-16libio: Add __nonnull for FILE * arguments of fclose and freopenXi Ruoyao1-3/+3
Calling fclose or freopen with a null FILE * is undefined behavior, and doing so in practice will cause a SIGSEGV. So it seems suitable for __nonnull. This will help the compiler to warn for some buggy code, like https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109570. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
2023-05-01Mark various cold functions as __COLDSergey Bugaev1-1/+1
GCC docs explicitly list perror () as a good candidate for using __attribute__ ((cold)). So apply __COLD to perror () and similar functions. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> Message-Id: <20230429131223.2507236-3-bugaevc@gmail.com>
2023-04-06<stdio.h>: Make fopencookie, vasprintf, asprintf available by defaultFlorian Weimer1-3/+3
FreeBSD makes these functions available by default, so we should not treat them as GNU-specific and restrict them to _GNU_SOURCE. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
2023-04-05libio: Remove unused pragma weak on vtableAdhemerval Zanella1-2/+0
Both _IO_file_jumps_alias and _IO_wfile_jumps_alias are defined as alias.
2023-03-28system: Add "--" after "-c" for sh (BZ #28519)Joe Simmons-Talbott1-1/+1
Prevent sh from interpreting a user string as shell options if it starts with '-' or '+'. Since the version of /bin/sh used for testing system() is different from the full-fledged system /bin/sh add support to it for handling "--" after "-c". Add a testcase to ensure the expected behavior. Signed-off-by: Joe Simmons-Talbott <josimmon@redhat.com> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
2023-03-27libio: Remove the usage of __libc_IO_vtablesAdhemerval Zanella Netto17-416/+620
Instead of using a special ELF section along with a linker script directive to put the IO vtables within the RELRO section, the libio vtables are all moved to an array marked as data.relro (so linker will place in the RELRO segment without the need of extra directives). To avoid static linking namespace issues and including all vtable referenced objects, all required function pointers are set to weak alias. Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu. Reviewed-by: Carlos O'Donell <carlos@redhat.com>
2023-03-27libio: Do not autogenerate stdio_lim.hAdhemerval Zanella Netto1-7/+12
Instead define the required fields in system dependend files. The only system dependent definition is FILENAME_MAX, which should match POSIX PATH_MAX, and it is obtained from either kernel UAPI or mach headers. Currently set pre-defined value from current kernels. It avoids a circular dependendy when including stdio.h in gen-as-const-headers files. Checked on x86_64-linux-gnu and i686-linux-gnu Reviewed-by: Carlos O'Donell <carlos@redhat.com>
2023-03-27Move libc_freeres_ptrs and libc_subfreeres to hidden/weak functionsAdhemerval Zanella Netto3-5/+5
They are both used by __libc_freeres to free all library malloc allocated resources to help tooling like mtrace or valgrind with memory leak tracking. The current scheme uses assembly markers and linker script entries to consolidate the free routine function pointers in the RELRO segment and to be freed buffers in BSS. This patch changes it to use specific free functions for libc_freeres_ptrs buffers and call the function pointer array directly with call_function_static_weak. It allows the removal of both the internal macros and the linker script sections. Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu. Reviewed-by: Carlos O'Donell <carlos@redhat.com>
2023-03-02C2x scanf binary constant handlingJoseph Myers3-11/+92
C2x adds binary integer constants starting with 0b or 0B, and supports those constants for the %i scanf format (in addition to the %b format, which isn't yet implemented for scanf in glibc). Implement that scanf support for glibc. As with the strtol support, this is incompatible with previous C standard versions, in that such an input string starting with 0b or 0B was previously required to be parsed as 0 (with the rest of the input potentially matching subsequent parts of the scanf format string). Thus this patch adds 12 new __isoc23_* functions per long double format (12, 24 or 36 depending on how many long double formats the glibc configuration supports), with appropriate header redirection support (generally very closely following that for the __isoc99_* scanf functions - note that __GLIBC_USE (DEPRECATED_SCANF) takes precedence over __GLIBC_USE (C2X_STRTOL), so the case of GNU extensions to C89 continues to get old-style GNU %a and does not get this new feature). The function names would remain as __isoc23_* even if C2x ends up published in 2024 rather than 2023. When scanf %b support is added, I think it will be appropriate for all versions of scanf to follow C2x rules for inputs to the %b format (given that there are no compatibility concerns for a new format). Tested for x86_64 (full glibc testsuite). The first version was also tested for powerpc (32-bit) and powerpc64le (stdio-common/ and wcsmbs/ tests), and with build-many-glibcs.py.
2023-02-14stdio: Do not ignore posix_spawn error on popen (BZ #29016)Adhemerval Zanella1-16/+22
To correctly return error in case of default shell is not present. Checked on x86_64-linux-gnu.
2023-02-06Replace rawmemchr (s, '\0') with strchrWilco Dijkstra1-1/+1
Almost all uses of rawmemchr find the end of a string. Since most targets use a generic implementation, replacing it with strchr is better since that is optimized by compilers into strlen (s) + s. Also fix the generic rawmemchr implementation to use a cast to unsigned char in the if statement. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
2023-01-31libio: Update number of written bytes in dprintf implementationFlorian Weimer1-0/+1
The __printf_buffer_flush_dprintf function needs to record that the buffer has been written before reusing it. Without this accounting, dprintf always returns zero. Fixes commit 8ece45e4f586abd212d1c02d74d38ef681a45600 ("libio: Convert __vdprintf_internal to buffers"). Reviewed-by: Carlos O'Donell <carlos@redhat.com> Tested-by: Carlos O'Donell <carlos@redhat.com>