[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 1/3] tests: Introduce global mock library



On 11.05.2016 16:50, Peter Krempa wrote:
> On Tue, May 10, 2016 at 17:24:12 +0200, Michal Privoznik wrote:
>> The intent is that this library is going to be called every time
>> to check if we are not touching anything outside srcdir or
>> builddir.
>>
>> Signed-off-by: Michal Privoznik <mprivozn redhat com>
>> ---
>>  cfg.mk                |   2 +-
>>  tests/Makefile.am     |  13 +++-
>>  tests/testutils.c     |   9 +++
>>  tests/testutils.h     |  10 +--
>>  tests/vircgroupmock.c |   6 +-
>>  tests/virpcimock.c    |   6 +-
>>  tests/virtestmock.c   | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  7 files changed, 215 insertions(+), 11 deletions(-)
>>  create mode 100644 tests/virtestmock.c
> 
> [...]
> 
> I'll have to look whether this is really necessary.
> 
>> diff --git a/tests/vircgroupmock.c b/tests/vircgroupmock.c
>> index cfc51e8..395254b 100644
>> --- a/tests/vircgroupmock.c
>> +++ b/tests/vircgroupmock.c
>> @@ -416,8 +416,10 @@ static void init_syms(void)
>>  
>>      LOAD_SYM(fopen);
>>      LOAD_SYM(access);
>> -    LOAD_SYM_ALT(lstat, __lxstat);
>> -    LOAD_SYM_ALT(stat, __xstat);
>> +    LOAD_SYM(lstat);
>> +    LOAD_SYM(__lxstat);
>> +    LOAD_SYM(stat);
>> +    LOAD_SYM(__xstat);
>>      LOAD_SYM(mkdir);
>>      LOAD_SYM(open);
>>  }
> 
> 
> LOAD_SYM_ALT is unused after this. Additionally this could be aggregated
> into a single header so that every mock library doesn't have to
> reimplement these helpers.

Oh, you mean that LOAD_SYM macro should go somewhere into a header file?
Well I can do that, but I'd rather do that in a separate patch, since
that would be touching more files than there are in this patch. Morevoer,
we have to think twice where to put it because as I was testing this
approach, I've encountered the following deadlock:

Thread 1 (Thread 0x7f2f61527800 (LWP 31448)):
#0  0x00007f2f5dbbbcac in __lll_lock_wait () from /lib64/libpthread.so.0
#1  0x00007f2f5dbb5aa5 in pthread_mutex_lock () from /lib64/libpthread.so.0
#2  0x00007f2f60c20885 in virMutexLock (m=0x7f2f61137920 <virLogMutex>) at util/virthread.c:89
#3  0x00007f2f60be5e2f in virLogLock () at util/virlog.c:143
#4  0x00007f2f60be692e in virLogSourceUpdate (source=0x7f2f611291f0 <virLogSelf>) at util/virlog.c:473
#5  0x00007f2f60be6bae in virLogVMessage (source=0x7f2f611291f0 <virLogSelf>, priority=VIR_LOG_ERROR, filename=0x7f2f60e534da "util/virfile.c", linenr=3157, funcname=0x7f2f60e544a8 <__FUNCTION__.16218> "virFilePrintf", metadata=0x7fffd2f20730, fmt=0x7f2f60e5140a "%s", 
    vargs=0x7fffd2f20560) at util/virlog.c:574
#6  0x00007f2f60be6ad8 in virLogMessage (source=0x7f2f611291f0 <virLogSelf>, priority=VIR_LOG_ERROR, filename=0x7f2f60e534da "util/virfile.c", linenr=3157, funcname=0x7f2f60e544a8 <__FUNCTION__.16218> "virFilePrintf", metadata=0x7fffd2f20730, fmt=0x7f2f60e5140a "%s")
    at util/virlog.c:519
#7  0x00007f2f60bc68eb in virRaiseErrorLog (filename=0x7f2f60e534da "util/virfile.c", funcname=0x7f2f60e544a8 <__FUNCTION__.16218> "virFilePrintf", linenr=3157, err=0x62bdc0, meta=0x7fffd2f20730) at util/virerror.c:671
#8  0x00007f2f60bc6c46 in virRaiseErrorFull (filename=0x7f2f60e534da "util/virfile.c", funcname=0x7f2f60e544a8 <__FUNCTION__.16218> "virFilePrintf", linenr=3157, domain=0, code=38, level=VIR_ERR_ERROR, str1=0x7f2f60e5140a "%s", 
    str2=0x7fffd2f20d00 "Could not write to stream: Bad file descriptor", str3=0x0, int1=9, int2=-1, fmt=0x7f2f60e5140a "%s") at util/virerror.c:766
#9  0x00007f2f60bc885c in virReportSystemErrorFull (domcode=0, theerrno=9, filename=0x7f2f60e534da "util/virfile.c", funcname=0x7f2f60e544a8 <__FUNCTION__.16218> "virFilePrintf", linenr=3157, fmt=0x7f2f60e5354f "%s") at util/virerror.c:1512
#10 0x00007f2f60bd16e3 in virFilePrintf (fp=0x7f2f5dba7680 <_IO_2_1_stderr_>, msg=0x7f2f6114bb1d "*** %s ***\n") at util/virfile.c:3156
#11 0x00007f2f6113b7c7 in checkPath (path=0x7f2f595a0011 "/etc/hosts") at virtestmock.c:60
#12 0x00007f2f6113b960 in fopen (path=0x7f2f595a0011 "/etc/hosts", mode=0x7f2f5959fff0 "rce") at virtestmock.c:90
#13 0x00007f2f5959b713 in internal_setent () from /lib64/libnss_files.so.2
#14 0x00007f2f5959c107 in _nss_files_gethostbyname4_r () from /lib64/libnss_files.so.2
#15 0x00007f2f5d8dea80 in gaih_inet () from /lib64/libc.so.6
#16 0x00007f2f5d8e0a1c in getaddrinfo () from /lib64/libc.so.6
#17 0x00007f2f60c29f5e in virGetHostnameImpl (quiet=true) at util/virutil.c:703
#18 0x00007f2f60c2a134 in virGetHostnameQuiet () at util/virutil.c:745
#19 0x00007f2f60be6848 in virLogHostnameString (rawmsg=0x7fffd2f21dc8, msg=0x7fffd2f21dd0) at util/virlog.c:449
#20 0x00007f2f60be6d7c in virLogVMessage (source=0x7f2f61129250 <virLogSelf>, priority=VIR_LOG_WARN, filename=0x7f2f60e534da "util/virfile.c", linenr=95, funcname=0x7f2f60e53f70 <__func__.15542> "virFileClose", metadata=0x0, 
    fmt=0x7f2f60e534e9 "Tried to close invalid fd %d", vargs=0x7fffd2f21e70) at util/virlog.c:610
#21 0x00007f2f60be6ad8 in virLogMessage (source=0x7f2f61129250 <virLogSelf>, priority=VIR_LOG_WARN, filename=0x7f2f60e534da "util/virfile.c", linenr=95, funcname=0x7f2f60e53f70 <__func__.15542> "virFileClose", metadata=0x0, 
    fmt=0x7f2f60e534e9 "Tried to close invalid fd %d") at util/virlog.c:519
#22 0x00007f2f60bcb124 in virFileClose (fdptr=0x7fffd2f223b8, flags=VIR_FILE_CLOSE_PRESERVE_ERRNO) at util/virfile.c:95
#23 0x0000000000404bca in virtTestCaptureProgramExecChild (argv=0x7fffd2f224d0, pipefd=5) at testutils.c:373
#24 0x0000000000404ce1 in virtTestCaptureProgramOutput (argv=0x7fffd2f224d0, buf=0x7fffd2f22490, maxlen=4096) at testutils.c:404
#25 0x0000000000402738 in testCompareOutputLit (expectData=0x406948 " Id    Name", ' ' <repeats 27 times>, "State\n", '-' <repeats 52 times>, "\n 1     test", ' ' <repeats 27 times>, "running\n\n", filter=0x0, argv=0x7fffd2f224d0) at virshtest.c:68
#26 0x000000000040281f in testCompareListDefault (data=0x0) at virshtest.c:105
#27 0x00000000004045f4 in virtTestRun (title=0x406d14 "virsh list (default)", body=0x4027bf <testCompareListDefault>, data=0x0) at testutils.c:175
#28 0x000000000040307d in mymain () at virshtest.c:261
#29 0x0000000000405f69 in virtTestMain (argc=1, argv=0x7fffd2f227a8, lib=0x0, func=0x402fff <mymain>) at testutils.c:967
#30 0x0000000000404485 in main (argc=1, argv=0x7fffd2f227a8) at virshtest.c:422

Guess what, fprintf is replaced by virFilePrintf(). I mean WAT?!

> 
>> diff --git a/tests/virpcimock.c b/tests/virpcimock.c
>> index cfe42a6..ac8a665 100644
>> --- a/tests/virpcimock.c
>> +++ b/tests/virpcimock.c
>> @@ -790,8 +790,10 @@ init_syms(void)
>>      } while (0)
>>  
>>      LOAD_SYM(access);
>> -    LOAD_SYM_ALT(lstat, __lxstat);
>> -    LOAD_SYM_ALT(stat, __xstat);
>> +    LOAD_SYM(lstat);
>> +    LOAD_SYM(__lxstat);
>> +    LOAD_SYM(stat);
>> +    LOAD_SYM(__xstat);
>>      LOAD_SYM(canonicalize_file_name);
>>      LOAD_SYM(open);
>>      LOAD_SYM(close);
> 
> Same as above in regards of LOAD_SYM_ALT.
> 
>> diff --git a/tests/virtestmock.c b/tests/virtestmock.c
>> new file mode 100644
>> index 0000000..f138e98
>> --- /dev/null
>> +++ b/tests/virtestmock.c
>> @@ -0,0 +1,180 @@
>> +/*
>> + * Copyright (C) 2014 Red Hat, Inc.
> 
> 2016?

Ooops.

> 
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library.  If not, see
>> + * <http://www.gnu.org/licenses/>.
>> + *
>> + * Author: Michal Privoznik <mprivozn redhat com>
>> + */
>> +
>> +#include <config.h>
>> +#include <unistd.h>
>> +#include <sys/types.h>
>> +#include <stdio.h>
>> +#include <dlfcn.h>
>> +#include <sys/stat.h>
>> +#include <fcntl.h>
>> +
>> +#include "internal.h"
>> +#include "configmake.h"
>> +
>> +static int (*realopen)(const char *path, int flags, ...);
>> +static FILE *(*realfopen)(const char *path, const char *mode);
>> +static int (*realaccess)(const char *path, int mode);
>> +static int (*realstat)(const char *path, struct stat *sb);
>> +static int (*real__xstat)(int ver, const char *path, struct stat *sb);
>> +static int (*reallstat)(const char *path, struct stat *sb);
>> +static int (*real__lxstat)(int ver, const char *path, struct stat *sb);
>> +
>> +static void init_syms(void)
>> +{
>> +    if (realopen)
>> +        return;
>> +
>> +#define LOAD_SYM(name)                                                 \
>> +   do {                                                                \
>> +       if (!(real ## name = dlsym(RTLD_NEXT, #name))) {                \
>> +           fprintf(stderr, "Cannot find real '%s' symbol\n", #name);   \
>> +           abort();                                                    \
>> +       }                                                               \
>> +   } while (0)
>> +
>> +#define LOAD_SYM_ALT(name1, name2)                                     \
>> +   do {                                                                \
>> +       if (!(real ## name1 = dlsym(RTLD_NEXT, #name1)) &&              \
>> +           !(real ## name2 = dlsym(RTLD_NEXT, #name2))) {              \
>> +           fprintf(stderr, "Cannot find real '%s' or '%s' symbol\n", #name1, #name2); \
>> +           abort();                                                    \
>> +       }                                                               \
>> +   } while (0)
> 
> This would too benefit from not having to reimplement these.
> 
>> +
>> +    LOAD_SYM(open);
>> +    LOAD_SYM(fopen);
>> +    LOAD_SYM(access);
>> +    LOAD_SYM_ALT(stat, __xstat);
>> +    LOAD_SYM_ALT(lstat, __lxstat);
>> +}
>> +
>> +static void
>> +checkPath(const char *path)
>> +{
>> +    if (!STRPREFIX(path, abs_topsrcdir) &&
>> +        !STRPREFIX(path, abs_topbuilddir)) {
>> +        /* Okay, this is a dummy check and spurious print.
>> +         * But this is going to be replaced soon. */
>> +        fprintf(stderr, "*** %s ***\n", path);
> 
> I'd rather do nothing at this place. Leave it empty until you implement
> it in the next patch.

Okay.

Michal


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]