From f196c9f1d69238a814ff3152103f3bd310efdf0d Mon Sep 17 00:00:00 2001 From: Dimitri van Heesch Date: Sat, 17 Oct 2015 16:38:29 +0200 Subject: Bug 756241 - Race condition in parallel DOT runs --- qtools/Doxyfile | 4 +-- src/dot.cpp | 82 ++++++++++++++++++++++++++------------------------------- src/dot.h | 46 +++++++++++++++++++++++++------- 3 files changed, 77 insertions(+), 55 deletions(-) diff --git a/qtools/Doxyfile b/qtools/Doxyfile index 41f2ad9..2943270 100644 --- a/qtools/Doxyfile +++ b/qtools/Doxyfile @@ -279,7 +279,7 @@ PERL_PATH = /usr/bin/perl CLASS_DIAGRAMS = YES MSCGEN_PATH = HIDE_UNDOC_RELATIONS = YES -HAVE_DOT = NO +HAVE_DOT = YES DOT_NUM_THREADS = 0 DOT_FONTNAME = DOT_FONTSIZE = 10 @@ -306,4 +306,4 @@ MAX_DOT_GRAPH_DEPTH = 0 DOT_TRANSPARENT = YES DOT_MULTI_TARGETS = NO GENERATE_LEGEND = YES -DOT_CLEANUP = NO +DOT_CLEANUP = YES diff --git a/src/dot.cpp b/src/dot.cpp index 701f868..2f245f6 100644 --- a/src/dot.cpp +++ b/src/dot.cpp @@ -659,10 +659,9 @@ static bool writeSVGFigureLink(FTextStream &out,const QCString &relPath, // since dot silently reproduces the input file when it does not // support the PNG format, we need to check the result. -static void checkDotResult(const QCString &imgName) +static void checkDotResult(const char *imgExt, const char *imgName) { - QCString imgExt = getDotImageExtension(); - if (imgExt=="png") + if (qstrcmp(imgExt,"png")==0) { FILE *f = portable_fopen(imgName,"rb"); if (f) @@ -675,19 +674,19 @@ static void checkDotResult(const QCString &imgName) err("Image `%s' produced by dot is not a valid PNG!\n" "You should either select a different format " "(DOT_IMAGE_FORMAT in the config file) or install a more " - "recent version of graphviz (1.7+)\n",imgName.data() + "recent version of graphviz (1.7+)\n",imgName ); } } else { - err("Could not read image `%s' generated by dot!\n",imgName.data()); + err("Could not read image `%s' generated by dot!\n",imgName); } fclose(f); } else { - err("Could not open image `%s' generated by dot!\n",imgName.data()); + err("Could not open image `%s' generated by dot!\n",imgName); } } } @@ -793,54 +792,46 @@ int DotNodeList::compareValues(const DotNode *n1,const DotNode *n2) const DotRunner::DotRunner(const QCString &file,const QCString &path, bool checkResult,const QCString &imageName) - : m_file(file), m_path(path), - m_checkResult(checkResult), m_imageName(imageName) -{ - static bool dotCleanUp = Config_getBool("DOT_CLEANUP"); - m_cleanUp = dotCleanUp; + : m_dotExe(Config_getString("DOT_PATH")+"dot"), + m_file(file), m_path(path), + m_checkResult(checkResult), m_imageName(imageName), + m_imgExt(getDotImageExtension()) +{ + static bool dotCleanUp = Config_getBool("DOT_CLEANUP"); + static bool dotMultiTargets = Config_getBool("DOT_MULTI_TARGETS"); + m_cleanUp = dotCleanUp; + m_multiTargets = dotMultiTargets; m_jobs.setAutoDelete(TRUE); } void DotRunner::addJob(const char *format,const char *output) { QCString args = QCString("-T")+format+" -o \""+output+"\""; - m_jobs.append(new QCString(args)); + m_jobs.append(new DotConstString(args)); } void DotRunner::addPostProcessing(const char *cmd,const char *args) { - m_postCmd = cmd; - m_postArgs = args; + m_postCmd.set(cmd); + m_postArgs.set(args); } bool DotRunner::run() { int exitCode=0; - // we need to use data here to make a copy of the string, as Config_getString can be called by - // multiple threads simulaneously and the reference counting is not thread safe. - QCString dotExe = Config_getString("DOT_PATH").data(); - dotExe+="dot"; - bool multiTargets = Config_getBool("DOT_MULTI_TARGETS"); QCString dotArgs; - QListIterator li(m_jobs); - QCString *s; - QCString file = m_file; - QCString path = m_path; - QCString imageName = m_imageName; - QCString postCmd = m_postCmd; - QCString postArgs = m_postArgs; - bool checkResult = m_checkResult; - bool cleanUp = m_cleanUp; - if (multiTargets) - { - dotArgs="\""+file+"\""; + QListIterator li(m_jobs); + DotConstString *s; + if (m_multiTargets) + { + dotArgs=QCString("\"")+m_file.data()+"\""; for (li.toFirst();(s=li.current());++li) { dotArgs+=' '; - dotArgs+=*s; + dotArgs+=s->data(); } - if ((exitCode=portable_system(dotExe,dotArgs,FALSE))!=0) + if ((exitCode=portable_system(m_dotExe.data(),dotArgs,FALSE))!=0) { goto error; } @@ -849,30 +840,33 @@ bool DotRunner::run() { for (li.toFirst();(s=li.current());++li) { - dotArgs="\""+file+"\" "+*s; - if ((exitCode=portable_system(dotExe,dotArgs,FALSE))!=0) + dotArgs=QCString("\"")+m_file.data()+"\" "+s->data(); + if ((exitCode=portable_system(m_dotExe.data(),dotArgs,FALSE))!=0) { goto error; } } } - if (!postCmd.isEmpty() && portable_system(postCmd,postArgs)!=0) + if (!m_postCmd.isEmpty() && portable_system(m_postCmd.data(),m_postArgs.data())!=0) { err("Problems running '%s' as a post-processing step for dot output\n",m_postCmd.data()); return FALSE; } - if (checkResult) checkDotResult(imageName); - if (cleanUp) + if (m_checkResult) + { + checkDotResult(m_imgExt.data(),m_imageName.data()); + } + if (m_cleanUp) { //printf("removing dot file %s\n",m_file.data()); //QDir(path).remove(file); - m_cleanupItem.file = file; - m_cleanupItem.path = path; + m_cleanupItem.file.set(m_file.data()); + m_cleanupItem.path.set(m_path.data()); } return TRUE; error: err("Problems running dot: exit code=%d, command='%s', arguments='%s'\n", - exitCode,dotExe.data(),dotArgs.data()); + exitCode,m_dotExe.data(),dotArgs.data()); return FALSE; } @@ -1202,7 +1196,7 @@ void DotWorkerThread::run() while ((runner=m_queue->dequeue())) { runner->run(); - DotRunner::CleanupItem cleanup = runner->cleanup(); + const DotRunner::CleanupItem &cleanup = runner->cleanup(); if (!cleanup.file.isEmpty()) { m_cleanupItems.append(new DotRunner::CleanupItem(cleanup)); @@ -1216,7 +1210,7 @@ void DotWorkerThread::cleanup() DotRunner::CleanupItem *ci; for (;(ci=it.current());++it) { - QDir(ci->path).remove(ci->file); + QDir(ci->path.data()).remove(ci->file.data()); } } @@ -4237,7 +4231,7 @@ void writeDotGraphFromFile(const char *inFile,const char *outDir, return; } - if (format==GOF_BITMAP) checkDotResult(absImgName); + if (format==GOF_BITMAP) checkDotResult(getDotImageExtension(),absImgName); Doxygen::indexList->addImageFile(imgName); diff --git a/src/dot.h b/src/dot.h index 64d6dd4..e12d547 100644 --- a/src/dot.h +++ b/src/dot.h @@ -326,6 +326,31 @@ class DotGroupCollaboration QList m_edges; }; +/** Minimal constant string class that is thread safe, once initialized. */ +class DotConstString +{ + public: + DotConstString() { m_str=0; } + ~DotConstString() { delete[] m_str; } + DotConstString(const QCString &s) : m_str(0) { set(s); } + DotConstString(const DotConstString &s) : m_str(0) { set(s.data()); } + const char *data() const { return m_str; } + bool isEmpty() const { return m_str==0 || m_str[0]=='\0'; } + void set(const QCString &s) + { + delete[] m_str; + m_str=0; + if (!s.isEmpty()) + { + m_str=new char[s.length()+1]; + qstrcpy(m_str,s.data()); + } + } + private: + DotConstString &operator=(const DotConstString &); + char *m_str; +}; + /** Helper class to run dot from doxygen. */ class DotRunner @@ -333,8 +358,8 @@ class DotRunner public: struct CleanupItem { - QCString path; - QCString file; + DotConstString path; + DotConstString file; }; /** Creates a runner for a dot \a file. */ @@ -352,16 +377,19 @@ class DotRunner /** Runs dot for all jobs added. */ bool run(); - CleanupItem cleanup() const { return m_cleanupItem; } + const CleanupItem &cleanup() const { return m_cleanupItem; } private: - QList m_jobs; - QCString m_postArgs; - QCString m_postCmd; - QCString m_file; - QCString m_path; + DotConstString m_dotExe; + bool m_multiTargets; + QList m_jobs; + DotConstString m_postArgs; + DotConstString m_postCmd; + DotConstString m_file; + DotConstString m_path; bool m_checkResult; - QCString m_imageName; + DotConstString m_imageName; + DotConstString m_imgExt; bool m_cleanUp; CleanupItem m_cleanupItem; }; -- cgit v0.12