Simplified the implementation of PcapNgFileWriterDevice opening. (#1804)

* Streamlined PcapNgFileWriterDevice to have all open configurations pass through a single point `openImpl`.

* Streamlined PcapNgFileReader's metadata accessors.

* Added check to ensure no append mode open can be made with new metadata.

* Added overrides to fix CI.

* Fixed cppcheck error about virtual call in destructor.

* Split openImpl to openWrite and openAppend.

* Reverted fetching of metadata to individual blocks.
Changed PcapNgMetadata to private struct of PcapNgFileWriterDevice.

Author: Dimi1010

Pcap++/header/PcapFileDevice.h

@@ -461,25 +461,9 @@ namespace pcpp
 		/// A destructor for this class
 		virtual ~PcapNgFileWriterDevice()
 		{
-			close();
+			PcapNgFileWriterDevice::close();
 		}
 
-		/// Open the file in a write mode. If file doesn't exist, it will be created. If it does exist it will be
-		/// overwritten, meaning all its current content will be deleted. As opposed to open(), this method also allows
-		/// writing several metadata attributes that will be stored in the header of the file
-		/// @param[in] os A string describing the operating system that was used to capture the packets. If this string
-		/// is empty or null it will be ignored
-		/// @param[in] hardware A string describing the hardware that was used to capture the packets. If this string is
-		/// empty or null it will be ignored
-		/// @param[in] captureApp A string describing the application that was used to capture the packets. If this
-		/// string is empty or null it will be ignored
-		/// @param[in] fileComment A string containing a user-defined comment that will be part of the metadata of the
-		/// file. If this string is empty or null it will be ignored
-		/// @return True if file was opened/created successfully or if file is already opened. False if opening the file
-		/// failed for some reason (an error will be printed to log)
-		bool open(const std::string& os, const std::string& hardware, const std::string& captureApp,
-		          const std::string& fileComment);
-
 		/// The pcap-ng format allows adding a user-defined comment for each stored packet. This method writes a
 		/// RawPacket to the file and adds a comment to it. Before using this method please verify the file is opened
 		/// using open(). This method won't change the written packet or the input comment
@@ -497,7 +481,7 @@ namespace pcpp
 		/// @param[in] packet A reference for an existing RawPcket to write to the file
 		/// @return True if a packet was written successfully. False will be returned if the file isn't opened (an error
 		/// will be printed to log)
-		bool writePacket(RawPacket const& packet);
+		bool writePacket(RawPacket const& packet) override;
 
 		/// Write multiple RawPacket to the file. Before using this method please verify the file is opened using
 		/// open(). This method won't change the written packets or the RawPacketVector instance
@@ -506,13 +490,13 @@ namespace pcpp
 		/// @return True if all packets were written successfully to the file. False will be returned if the file isn't
 		/// opened (also, an error log will be printed) or if at least one of the packets wasn't written successfully to
 		/// the file
-		bool writePackets(const RawPacketVector& packets);
+		bool writePackets(const RawPacketVector& packets) override;
 
 		/// Open the file in a write mode. If file doesn't exist, it will be created. If it does exist it will be
 		/// overwritten, meaning all its current content will be deleted
 		/// @return True if file was opened/created successfully or if file is already opened. False if opening the file
 		/// failed for some reason (an error will be printed to log)
-		bool open();
+		bool open() override;
 
 		/// Same as open(), but enables to open the file in append mode in which packets will be appended to the file
 		/// instead of overwrite its current content. In append mode file must exist, otherwise opening will fail
@@ -521,23 +505,58 @@ namespace pcpp
 		/// @return True of managed to open the file successfully. In case appendMode is set to true, false will be
 		/// returned if file wasn't found or couldn't be read, if file type is not pcap-ng. In case appendMode is set to
 		/// false, please refer to open() for return values
-		bool open(bool appendMode);
+		bool open(bool appendMode) override;
+
+		/// Open the file in a write mode. If file doesn't exist, it will be created. If it does exist it will be
+		/// overwritten, meaning all its current content will be deleted. As opposed to open(), this method also allows
+		/// writing several metadata attributes that will be stored in the header of the file
+		/// @param[in] os A string describing the operating system that was used to capture the packets. If this string
+		/// is empty or null it will be ignored
+		/// @param[in] hardware A string describing the hardware that was used to capture the packets. If this string is
+		/// empty or null it will be ignored
+		/// @param[in] captureApp A string describing the application that was used to capture the packets. If this
+		/// string is empty or null it will be ignored
+		/// @param[in] fileComment A string containing a user-defined comment that will be part of the metadata of the
+		/// file. If this string is empty or null it will be ignored
+		/// @return True if file was opened/created successfully or if file is already opened. False if opening the file
+		/// failed for some reason (an error will be printed to log)
+		bool open(const std::string& os, const std::string& hardware, const std::string& captureApp,
+		          const std::string& fileComment);
 
 		/// Flush packets to the pcap-ng file
 		void flush();
 
 		/// Flush and close the pcap-ng file
-		void close();
+		void close() override;
 
 		/// Get statistics of packets written so far.
 		/// @param[out] stats The stats struct where stats are returned
-		void getStatistics(PcapStats& stats) const;
+		void getStatistics(PcapStats& stats) const override;
 
 		/// Set a filter for PcapNG writer device. Only packets that match the filter will be persisted
 		/// @param[in] filterAsString The filter to be set in Berkeley Packet Filter (BPF) syntax
 		/// (http://biot.com/capstats/bpf.html)
 		/// @return True if filter set successfully, false otherwise
-		bool setFilter(std::string filterAsString);
+		bool setFilter(std::string filterAsString) override;
+
+	private:
+		/// @struct PcapNgMetadata
+		/// @brief A struct for holding the metadata of a pcap-ng file. The metadata includes the operating system,
+		/// hardware, capture application and file comment.
+		struct PcapNgMetadata
+		{
+			/// The operating system that was used for capturing the packets
+			std::string os;
+			/// The hardware that was used for capturing the packets
+			std::string hardware;
+			/// The capture application that was used for capturing the packets
+			std::string captureApplication;
+			/// The comment that was written inside the file
+			std::string comment;
+		};
+
+		bool openWrite(PcapNgMetadata const* metadata = nullptr);
+		bool openAppend();
 	};
 
 }  // namespace pcpp

Pcap++/src/PcapFileDevice.cpp

@@ -492,75 +492,59 @@ namespace pcpp
 		if (m_LightPcapNg == nullptr)
 		{
 			PCPP_LOG_ERROR("Pcapng file device '" << m_FileName << "' not opened");
-			return "";
+			return {};
 		}
 
 		light_pcapng_file_info* fileInfo = light_pcang_get_file_info(toLightPcapNgT(m_LightPcapNg));
-		if (fileInfo == nullptr)
-			return "";
-		char* res = fileInfo->os_desc;
-		size_t len = fileInfo->os_desc_size;
-		if (len == 0 || res == nullptr)
-			return "";
-
-		return std::string(res, len);
+		if (fileInfo == nullptr || fileInfo->os_desc == nullptr || fileInfo->os_desc_size == 0)
+			return {};
+
+		return std::string(fileInfo->os_desc, fileInfo->os_desc_size);
 	}
 
 	std::string PcapNgFileReaderDevice::getHardware() const
 	{
 		if (m_LightPcapNg == nullptr)
 		{
 			PCPP_LOG_ERROR("Pcapng file device '" << m_FileName << "' not opened");
-			return "";
+			return {};
 		}
 
 		light_pcapng_file_info* fileInfo = light_pcang_get_file_info(toLightPcapNgT(m_LightPcapNg));
-		if (fileInfo == nullptr)
-			return "";
-		char* res = fileInfo->hardware_desc;
-		size_t len = fileInfo->hardware_desc_size;
-		if (len == 0 || res == nullptr)
-			return "";
-
-		return std::string(res, len);
+		if (fileInfo == nullptr || fileInfo->hardware_desc == nullptr || fileInfo->hardware_desc_size == 0)
+			return {};
+
+		return std::string(fileInfo->hardware_desc, fileInfo->hardware_desc_size);
 	}
 
 	std::string PcapNgFileReaderDevice::getCaptureApplication() const
 	{
 		if (m_LightPcapNg == nullptr)
 		{
 			PCPP_LOG_ERROR("Pcapng file device '" << m_FileName << "' not opened");
-			return "";
+			return {};
 		}
 
 		light_pcapng_file_info* fileInfo = light_pcang_get_file_info(toLightPcapNgT(m_LightPcapNg));
-		if (fileInfo == nullptr)
-			return "";
-		char* res = fileInfo->user_app_desc;
-		size_t len = fileInfo->user_app_desc_size;
-		if (len == 0 || res == nullptr)
-			return "";
-
-		return std::string(res, len);
+		if (fileInfo == nullptr || fileInfo->user_app_desc == nullptr || fileInfo->user_app_desc_size == 0)
+			return {};
+
+		return std::string(fileInfo->user_app_desc, fileInfo->user_app_desc_size);
 	}
 
 	std::string PcapNgFileReaderDevice::getCaptureFileComment() const
 	{
 		if (m_LightPcapNg == nullptr)
 		{
 			PCPP_LOG_ERROR("Pcapng file device '" << m_FileName << "' not opened");
-			return "";
+			return {};
 		}
 
 		light_pcapng_file_info* fileInfo = light_pcang_get_file_info(toLightPcapNgT(m_LightPcapNg));
-		if (fileInfo == nullptr)
-			return "";
-		char* res = fileInfo->file_comment;
-		size_t len = fileInfo->file_comment_size;
-		if (len == 0 || res == nullptr)
-			return "";
-
-		return std::string(res, len);
+		if (fileInfo == nullptr || fileInfo->file_comment == nullptr || fileInfo->file_comment_size == 0)
+			return {};
+
+		return std::string(fileInfo->file_comment, fileInfo->file_comment_size);
 	}
 
 	// ~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -867,38 +851,6 @@ namespace pcpp
 		m_CompressionLevel = compressionLevel;
 	}
 
-	bool PcapNgFileWriterDevice::open(const std::string& os, const std::string& hardware, const std::string& captureApp,
-	                                  const std::string& fileComment)
-	{
-		if (m_LightPcapNg != nullptr)
-		{
-			PCPP_LOG_DEBUG("Pcap-ng descriptor already opened. Nothing to do");
-			return true;
-		}
-
-		m_NumOfPacketsNotWritten = 0;
-		m_NumOfPacketsWritten = 0;
-
-		light_pcapng_file_info* info =
-		    light_create_file_info(os.c_str(), hardware.c_str(), captureApp.c_str(), fileComment.c_str());
-
-		m_LightPcapNg = toLightPcapNgHandle(light_pcapng_open_write(m_FileName.c_str(), info, m_CompressionLevel));
-		if (m_LightPcapNg == nullptr)
-		{
-			PCPP_LOG_ERROR("Error opening file writer device for file '"
-			               << m_FileName << "': light_pcapng_open_write returned nullptr");
-
-			light_free_file_info(info);
-
-			m_DeviceOpened = false;
-			return false;
-		}
-
-		m_DeviceOpened = true;
-		PCPP_LOG_DEBUG("pcap-ng writer device for file '" << m_FileName << "' opened successfully");
-		return true;
-	}
-
 	bool PcapNgFileWriterDevice::writePacket(RawPacket const& packet, const std::string& comment)
 	{
 		if (m_LightPcapNg == nullptr)
@@ -955,6 +907,30 @@ namespace pcpp
 
 	bool PcapNgFileWriterDevice::open()
 	{
+		return openWrite();
+	}
+
+	bool PcapNgFileWriterDevice::open(bool appendMode)
+	{
+		return appendMode ? openAppend() : openWrite();
+	}
+
+	bool PcapNgFileWriterDevice::open(const std::string& os, const std::string& hardware, const std::string& captureApp,
+	                                  const std::string& fileComment)
+	{
+		PcapNgMetadata metadata;
+		metadata.os = os;
+		metadata.hardware = hardware;
+		metadata.captureApplication = captureApp;
+		metadata.comment = fileComment;
+		return openWrite(&metadata);
+	}
+
+	bool PcapNgFileWriterDevice::openWrite(PcapNgMetadata const* metadata)
+	{
+		// TODO: Ambiguity in the API
+		//   If the user calls open() and then open(true) - should we close the first one or report failure?
+		//   Currently the method reports a success, but the opened device would not match the appendMode.
 		if (m_LightPcapNg != nullptr)
 		{
 			PCPP_LOG_DEBUG("Pcap-ng descriptor already opened. Nothing to do");
@@ -964,7 +940,16 @@ namespace pcpp
 		m_NumOfPacketsNotWritten = 0;
 		m_NumOfPacketsWritten = 0;
 
-		light_pcapng_file_info* info = light_create_default_file_info();
+		light_pcapng_file_info* info;
+		if (metadata == nullptr)
+		{
+			info = light_create_default_file_info();
+		}
+		else
+		{
+			info = light_create_file_info(metadata->os.c_str(), metadata->hardware.c_str(),
+			                              metadata->captureApplication.c_str(), metadata->comment.c_str());
+		}
 
 		m_LightPcapNg = toLightPcapNgHandle(light_pcapng_open_write(m_FileName.c_str(), info, m_CompressionLevel));
 		if (m_LightPcapNg == nullptr)
@@ -983,10 +968,16 @@ namespace pcpp
 		return true;
 	}
 
-	bool PcapNgFileWriterDevice::open(bool appendMode)
+	bool PcapNgFileWriterDevice::openAppend()
 	{
-		if (!appendMode)
-			return open();
+		// TODO: Ambiguity in the API
+		//   If the user calls open() and then open(true) - should we close the first one or report failure?
+		//   Currently the method reports a success, but the opened device would not match the appendMode.
+		if (m_LightPcapNg != nullptr)
+		{
+			PCPP_LOG_DEBUG("Pcap-ng descriptor already opened. Nothing to do");
+			return true;
+		}
 
 		m_NumOfPacketsNotWritten = 0;
 		m_NumOfPacketsWritten = 0;