Date: Fri, 11 Oct 2013 21:24:33 +0000 (UTC) From: Alan Somers <asomers@FreeBSD.org> To: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: svn commit: r256349 - projects/zfsd/head/cddl/sbin/zfsd Message-ID: <201310112124.r9BLOXn4024328@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: asomers Date: Fri Oct 11 21:24:33 2013 New Revision: 256349 URL: http://svnweb.freebsd.org/changeset/base/256349 Log: Miscellaneous bug fixes in zfsd * Created a new abstract Reader class that provides a common interface to read from both file descriptors and std:istreams, implemented as separate derived classes FDReader and IstreamReader. Changed EventBuffer to get its input from a Reader. Changed CaseFile::DeSerializeFile to open case files as ifstreams instead of as file descriptors. * Eliminated ZFSDaemon::ConnectToDevd's call to set the socket to nonblocking mode. Instead, use an API that never attempts to read more data than is available. EventBuffer::Fill() no longer ignore EAGAIN, which shouldn't happen now that the socket blocks. * Fix two file leaks * Fix file leak in CaseFile::Serialize(): open() without close() * Fix potential file leak in DeSerializeFile(): The exception handler for ZfsdException did not close the open case file. This code path was probably not exercised. * Fix four memory leaks * CaseFile::DeSerialize() was freeing its dirent structure when it had entries, but not when the directory was empty * zfsd did not purge case files on exit. This isn't really a problem because the memory won't leak until the program quits, but it clutters valgrind's output. * ZfsDaemon::RescanSystem() wasn't deleting newly created events after processing them. * DeSerializeFile(): an ifstream was closed() instead of deleted. * Miscellaneous style changes. Submitted by: asomers Approved by: ken (mentor) Sponsored by: Spectra Logic Corporation Modified: projects/zfsd/head/cddl/sbin/zfsd/case_file.cc projects/zfsd/head/cddl/sbin/zfsd/zfsd.cc projects/zfsd/head/cddl/sbin/zfsd/zfsd.h Modified: projects/zfsd/head/cddl/sbin/zfsd/case_file.cc ============================================================================== --- projects/zfsd/head/cddl/sbin/zfsd/case_file.cc Fri Oct 11 21:23:44 2013 (r256348) +++ projects/zfsd/head/cddl/sbin/zfsd/case_file.cc Fri Oct 11 21:24:33 2013 (r256349) @@ -40,6 +40,7 @@ */ #include <dirent.h> #include <iomanip> +#include <fstream> #include <sstream> #include <syslog.h> #include <unistd.h> @@ -53,6 +54,7 @@ /*============================ Namespace Control =============================*/ using std::auto_ptr; using std::hex; +using std::ifstream; using std::stringstream; using std::setfill; using std::setw; @@ -116,8 +118,12 @@ CaseFile::DeSerialize() int numCaseFiles(scandir(s_caseFilePath.c_str(), &caseFiles, DeSerializeSelector, /*compar*/NULL)); - if (numCaseFiles == 0 || numCaseFiles == -1) + if (numCaseFiles == -1) return; + if (numCaseFiles == 0) { + free(caseFiles); + return; + } for (int i = 0; i < numCaseFiles; i++) { @@ -472,7 +478,7 @@ CaseFile::DeSerializeFile(const char *fi string evString; CaseFile *existingCaseFile(NULL); CaseFile *caseFile(NULL); - int fd(-1); + ifstream *caseStream(NULL); try { uintmax_t poolGUID; @@ -519,28 +525,30 @@ CaseFile::DeSerializeFile(const char *fi */ caseFile = new CaseFile(Vdev(zpl.front(), vdevConf)); } - - fd = open(fullName.c_str(), O_RDONLY); - if (fd == -1) { + + caseStream = new ifstream(fullName.c_str()); + if ( (caseStream == NULL) || (! *caseStream) ) { throw ZfsdException("CaseFile::DeSerialize: Unable to " "read %s.\n", fileName); return; } + IstreamReader caseReader(caseStream); /* Re-load EventData */ - EventBuffer eventBuffer(fd); + EventBuffer eventBuffer(caseReader); while (eventBuffer.ExtractEvent(evString)) { DevCtlEvent *event(DevCtlEvent::CreateEvent(evString)); if (event != NULL) caseFile->m_events.push_back(event); } - close(fd); + delete caseStream; } catch (const ParseException &exp) { exp.Log(evString); if (caseFile != existingCaseFile) delete caseFile; - close(fd); + if (caseStream) + delete caseStream; /* * Since we can't parse the file, unlink it so we don't @@ -552,6 +560,8 @@ CaseFile::DeSerializeFile(const char *fi zfsException.Log(); if (caseFile != existingCaseFile) delete caseFile; + if (caseStream) + delete caseStream; } } @@ -632,6 +642,7 @@ CaseFile::Serialize() write(fd, eventString.c_str(), eventString.length()); } + close(fd); } void Modified: projects/zfsd/head/cddl/sbin/zfsd/zfsd.cc ============================================================================== --- projects/zfsd/head/cddl/sbin/zfsd/zfsd.cc Fri Oct 11 21:23:44 2013 (r256348) +++ projects/zfsd/head/cddl/sbin/zfsd/zfsd.cc Fri Oct 11 21:24:33 2013 (r256349) @@ -43,6 +43,7 @@ #include <sys/cdefs.h> #include <sys/disk.h> +#include <sys/filio.h> #include <sys/param.h> #include <sys/poll.h> #include <sys/socket.h> @@ -86,6 +87,32 @@ const char g_devdSock[] = "/var/ru int g_debug = 0; libzfs_handle_t *g_zfsHandle; +/*-------------------------------- FDReader -------------------------------*/ +//- FDReader Public Methods ---------------------------------------------------- +size_t +FDReader::in_avail() const +{ + int bytes; + if (ioctl(m_fd, FIONREAD, &bytes)) { + syslog(LOG_ERR, "ioctl FIONREAD: %s", strerror(errno)); + return (0); + } + return (bytes); +} + + +/*-------------------------------- IstreamReader ---------------------------*/ +//- IstreamReader Public Methods ---------------------------------------------- +ssize_t +IstreamReader::read(char* buf, size_t count) +{ + m_stream->read(buf, count); + if (m_stream->fail()) + return (-1); + return (m_stream->gcount()); +} + + /*-------------------------------- EventBuffer -------------------------------*/ //- EventBuffer Static Data ---------------------------------------------------- /** @@ -104,8 +131,8 @@ const char EventBuffer::s_eventEndTokens const char EventBuffer::s_keyPairSepTokens[] = " \t\n"; //- EventBuffer Public Methods ------------------------------------------------- -EventBuffer::EventBuffer(int fd) - : m_fd(fd), +EventBuffer::EventBuffer(Reader& reader) + : m_reader(reader), m_validLen(0), m_parsedLen(0), m_nextEventOffset(0), @@ -187,7 +214,8 @@ EventBuffer::ExtractEvent(string &eventS bool EventBuffer::Fill() { - ssize_t result; + size_t avail; + ssize_t consumed(0); /* Compact the buffer. */ if (m_nextEventOffset != 0) { @@ -199,19 +227,26 @@ EventBuffer::Fill() } /* Fill any empty space. */ - result = read(m_fd, m_buf + m_validLen, MAX_READ_SIZE - m_validLen); - if (result == -1) { - if (errno == EINTR || errno == EAGAIN) { - return (false); - } else { - err(1, "Read from devd socket failed"); + avail = m_reader.in_avail(); + if (avail) { + size_t want; + + want = std::min(avail, MAX_READ_SIZE - m_validLen); + consumed = m_reader.read(m_buf + m_validLen, want); + if (consumed == -1) { + if (errno == EINTR) { + return (false); + } else { + err(1, "EventBuffer::Fill(): Read failed"); + } } } - m_validLen += result; + + m_validLen += consumed; /* Guarantee our buffer is always NUL terminated. */ m_buf[m_validLen] = '\0'; - return (result > 0); + return (consumed > 0); } /*--------------------------------- ZfsDaemon --------------------------------*/ @@ -220,6 +255,7 @@ bool ZfsDaemon::s_logCaseFil bool ZfsDaemon::s_terminateEventLoop; char ZfsDaemon::s_pidFilePath[] = "/var/run/zfsd.pid"; pidfh *ZfsDaemon::s_pidFH; +FDReader* ZfsDaemon::s_reader; int ZfsDaemon::s_devdSockFD = -1; int ZfsDaemon::s_signalPipeFD[2]; bool ZfsDaemon::s_systemRescanRequested(false); @@ -306,6 +342,7 @@ ZfsDaemon::Init() void ZfsDaemon::Fini() { + PurgeCaseFiles(); ClosePIDFile(); } @@ -387,10 +424,9 @@ ZfsDaemon::ConnectToDevd() return (false); } - /* Don't block on reads. */ - if (fcntl(s_devdSockFD, F_SETFL, O_NONBLOCK) == -1) - err(1, "Unable to enable nonblocking behavior on devd socket"); + /* Connect the stream to the file descriptor */ + s_reader = new FDReader(s_devdSockFD); syslog(LOG_INFO, "Connection to devd successful"); return (true); } @@ -398,7 +434,10 @@ ZfsDaemon::ConnectToDevd() void ZfsDaemon::DisconnectFromDevd() { + delete s_reader; + s_reader = NULL; close(s_devdSockFD); + s_devdSockFD = -1; } void @@ -448,8 +487,8 @@ ZfsDaemon::FlushEvents() { char discardBuf[256]; - while (read(s_devdSockFD, discardBuf, sizeof(discardBuf)) > 0) - ; + while (s_reader->in_avail()) + s_reader->read(discardBuf, sizeof(discardBuf)); } bool @@ -528,6 +567,7 @@ ZfsDaemon::RescanSystem() event = DevCtlEvent::CreateEvent(evString); if (event != NULL) event->Process(); + delete event; } } } @@ -561,7 +601,7 @@ ZfsDaemon::DetectMissedEvents() void ZfsDaemon::EventLoop() { - EventBuffer eventBuffer(s_devdSockFD); + EventBuffer eventBuffer(*s_reader); while (s_terminateEventLoop == false) { struct pollfd fds[2]; Modified: projects/zfsd/head/cddl/sbin/zfsd/zfsd.h ============================================================================== --- projects/zfsd/head/cddl/sbin/zfsd/zfsd.h Fri Oct 11 21:23:44 2013 (r256348) +++ projects/zfsd/head/cddl/sbin/zfsd/zfsd.h Fri Oct 11 21:24:33 2013 (r256349) @@ -42,6 +42,7 @@ #define _ZFSD_H_ #include <cstdarg> +#include <iostream> #include <list> #include <map> #include <utility> @@ -57,6 +58,7 @@ using std::auto_ptr; using std::map; using std::pair; +using std::istream; using std::string; /*================================ Global Data ===============================*/ @@ -74,21 +76,131 @@ typedef int LeafIterFunc(zpool_handle_t #define NUM_ELEMENTS(x) (sizeof(x) / sizeof(*x)) /*============================= Class Definitions ============================*/ + +/*-------------------------------- Reader -------------------------------*/ +/** + * \brief A class that presents a common interface to both file descriptors and + * istreams . + * + * Standard C++ provides no way to create an iostream from a file descriptor or + * a FILE. The GNU, Apache, HPUX, and Solaris C++ libraries all provide + * non-standard ways to construct such a stream using similar semantics, but + * LLVM does not. Therefore this class is needed to ensure that zfsd can + * compile under LLVM. This class supports only the functionality needed by + * ZFSD; it does not implement the iostream API. + */ +class Reader +{ +public: + /** + * \brief Return the number of bytes immediately available for reading + */ + virtual size_t in_avail() const = 0; + + /** + * \brief Reads up to count bytes + * + * Whether this call blocks depends on the underlying input source. + * On error, -1 is returned, and errno will be set by the underlying + * source. + * + * \param buf Destination for the data + * \param count Maximum amount of data to read + * \returns Amount of data that was actually read + */ + virtual ssize_t read(char* buf, size_t count) = 0; +}; + + +/*-------------------------------- FDReader -------------------------------*/ +/** + * \brief Specialization of Reader that uses a file descriptor + */ +class FDReader : public Reader +{ +public: + /** + * \brief Constructor + * + * \param fd An open file descriptor. It will not be garbage + * collected by the destructor. + */ + FDReader(int fd); + + virtual size_t in_avail() const; + + virtual ssize_t read(char* buf, size_t count); + +protected: + /** Copy of the underlying file descriptor */ + int m_fd; +}; + +//- FDReader Inline Public Methods ----------------------------------------- +inline FDReader::FDReader(int fd) + : m_fd(fd) +{ +} + +inline ssize_t +FDReader::read(char* buf, size_t count) +{ + return (::read(m_fd, buf, count)); +} + + +/*-------------------------------- IstreamReader------------------------------*/ +/** + * \brief Specialization of Reader that uses a std::istream + */ +class IstreamReader : public Reader +{ +public: + /** + * Constructor + * + * \param stream Pointer to an open istream. It will not be + * garbage collected by the destructor. + */ + IstreamReader(istream* stream); + + virtual size_t in_avail() const; + + virtual ssize_t read(char* buf, size_t count); + +protected: + /** Copy of the underlying stream */ + istream* m_stream; +}; + +//- IstreamReader Inline Public Methods ---------------------------------------- +inline IstreamReader::IstreamReader(istream* stream) + : m_stream(stream) +{ +} + +inline size_t +IstreamReader::in_avail() const +{ + return (m_stream->rdbuf()->in_avail()); +} + + /*-------------------------------- EventBuffer -------------------------------*/ /** - * \brief Class buffering event data from Devd and splitting it - * into individual event strings. + * \brief Class buffering event data from Devd or a similar source and + * splitting it into individual event strings. * - * Users of this class initialize it with the file descriptor associated - * with the unix domain socket connection with devd. The lifetime of - * an EventBuffer instance should match that of the file descriptor passed - * to it. This is required as data from partially received events is - * retained in the EventBuffer in order to allow reconstruction of these - * events across multiple reads of the Devd file descriptor. + * Users of this class initialize it with a Reader associated with the unix + * domain socket connection with devd or a compatible source. The lifetime of + * an EventBuffer instance should match that of the Reader passed to it. This + * is required as data from partially received events is retained in the + * EventBuffer in order to allow reconstruction of these events across multiple + * reads of the stream. * - * Once the program determines that the Devd file descriptor is ready - * for reading, the EventBuffer::ExtractEvent() should be called in a - * loop until the method returns false. + * Once the program determines that the Reader is ready for reading, the + * EventBuffer::ExtractEvent() should be called in a loop until the method + * returns false. */ class EventBuffer { @@ -96,9 +208,9 @@ public: /** * Constructor * - * \param fd The file descriptor on which to buffer/parse event data. + * \param reader The data source on which to buffer/parse event data. */ - EventBuffer(int fd); + EventBuffer(Reader& reader); /** * Pull a single event string out of the event buffer. @@ -163,8 +275,8 @@ private: /** Temporary space for event data during our parsing. */ char m_buf[EVENT_BUFSIZE]; - /** Copy of the file descriptor linked to devd's domain socket. */ - int m_fd; + /** Reference to the reader linked to devd's domain socket. */ + Reader& m_reader; /** Valid bytes in m_buf. */ size_t m_validLen; @@ -362,6 +474,11 @@ private: static int s_devdSockFD; /** + * Reader object used by the EventBuffer + */ + static FDReader* s_reader; + + /** * Pipe file descriptors used to close races with our * signal handlers. */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201310112124.r9BLOXn4024328>