diff --git a/connections/implementation/internal_payload_factory.cc b/connections/implementation/internal_payload_factory.cc index 382740c0..014681eb 100644 --- a/connections/implementation/internal_payload_factory.cc +++ b/connections/implementation/internal_payload_factory.cc @@ -76,7 +76,7 @@ class BytesInternalPayload : public InternalPayload { } ExceptionOr SkipToOffset(size_t offset) override { - NEARBY_LOGS(WARNING) << "Bytes payload does not support offsets"; + LOG(WARNING) << "Bytes payload does not support offsets"; return {Exception::kIo}; } @@ -115,8 +115,8 @@ class OutgoingStreamInternalPayload : public InternalPayload { ByteArray scoped_bytes_read = std::move(bytes_read.result()); if (scoped_bytes_read.Empty()) { - NEARBY_LOGS(INFO) << "No more data for outgoing payload " << this - << ", closing InputStream."; + LOG(INFO) << "No more data for outgoing payload " << this + << ", closing InputStream."; input_stream->Close(); return {}; @@ -142,9 +142,8 @@ class OutgoingStreamInternalPayload : public InternalPayload { if (!real_offset.ok()) { return real_offset; } - NEARBY_LOGS(WARNING) << "Skip offset: " << real_offset.GetResult() - << ", expected offset: " << offset << " for payload " - << this; + LOG(WARNING) << "Skip offset: " << real_offset.GetResult() + << ", expected offset: " << offset << " for payload " << this; return {Exception::kIo}; } @@ -172,8 +171,8 @@ class IncomingStreamInternalPayload : public InternalPayload { Exception AttachNextChunk(const ByteArray& chunk) override { if (chunk.Empty()) { - NEARBY_LOGS(INFO) << "Received null last chunk for incoming payload " - << this << ", closing OutputStream."; + LOG(INFO) << "Received null last chunk for incoming payload " << this + << ", closing OutputStream."; Close(); return {Exception::kSuccess}; } @@ -182,8 +181,7 @@ class IncomingStreamInternalPayload : public InternalPayload { } ExceptionOr SkipToOffset(size_t offset) override { - NEARBY_LOGS(WARNING) << "Cannot skip offset for an incoming Payload " - << this; + LOG(WARNING) << "Cannot skip offset for an incoming Payload " << this; return {Exception::kIo}; } @@ -234,7 +232,7 @@ class OutgoingFileInternalPayload : public InternalPayload { } ExceptionOr SkipToOffset(size_t offset) override { - NEARBY_LOGS(INFO) << "SkipToOffset " << offset; + LOG(INFO) << "SkipToOffset " << offset; InputFile* file = payload_.AsFile(); if (!file) { return {Exception::kIo}; @@ -249,9 +247,9 @@ class OutgoingFileInternalPayload : public InternalPayload { if (!real_offset.ok()) { return real_offset; } - NEARBY_LOGS(WARNING) << "Skip offset: " << real_offset.GetResult() - << ", expected offset: " << offset - << " for file payload " << this; + LOG(WARNING) << "Skip offset: " << real_offset.GetResult() + << ", expected offset: " << offset << " for file payload " + << this; return {Exception::kIo}; } @@ -294,8 +292,7 @@ class IncomingFileInternalPayload : public InternalPayload { } ExceptionOr SkipToOffset(size_t offset) override { - NEARBY_LOGS(WARNING) << "Cannot skip offset for an incoming file Payload " - << this; + LOG(WARNING) << "Cannot skip offset for an incoming file Payload " << this; return {Exception::kIo}; } @@ -399,8 +396,8 @@ ErrorOr> CreateIncomingInternalPayload( } else { // This is an error condition, we don't have any way to generate a // file name for the output file. - NEARBY_LOGS(ERROR) << "File name not found in incoming file Payload, " - "and the Id wasn't found."; + LOG(ERROR) << "File name not found in incoming file Payload, " + "and the Id wasn't found."; return {Error(OperationResultCode::IO_FILE_OPENING_ERROR)}; } } @@ -413,14 +410,24 @@ ErrorOr> CreateIncomingInternalPayload( // there will be no input file to open. // On Chrome the file path should be empty, so use the payload id. if (ImplementationPlatform::GetCurrentOS() == OSName::kChromeOS) { + OutputFile output_file(payload_id); + if (!output_file.IsValid()) { + LOG(ERROR) << "Output file payload ID is not valid: " << payload_id; + return {Error(OperationResultCode::IO_FILE_OPENING_ERROR)}; + } return {std::make_unique( Payload(payload_id, InputFile(payload_id, total_size)), - OutputFile(payload_id), total_size)}; + std::move(output_file), total_size)}; } else { + OutputFile output_file(file_path); + if (!output_file.IsValid()) { + LOG(ERROR) << "Output file payload path is not valid: " << file_path; + return {Error(OperationResultCode::IO_FILE_OPENING_ERROR)}; + } return {std::make_unique( Payload(payload_id, parent_folder, file_name, InputFile(file_path, total_size)), - OutputFile(file_path), total_size)}; + std::move(output_file), total_size)}; } } default: diff --git a/connections/implementation/internal_payload_factory_test.cc b/connections/implementation/internal_payload_factory_test.cc index 46c5c923..c0aadbd6 100644 --- a/connections/implementation/internal_payload_factory_test.cc +++ b/connections/implementation/internal_payload_factory_test.cc @@ -132,7 +132,7 @@ TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromStreamMessage) { TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromFileMessage) { PayloadTransferFrame frame; - std::string path = "C:\\Downloads"; + std::string path = "/tmp/Downloads"; frame.set_packet_type(PayloadTransferFrame::DATA); auto& header = *frame.mutable_payload_header(); header.set_type(PayloadTransferFrame::PayloadHeader::FILE); @@ -153,7 +153,7 @@ TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromFileMessage) { TEST(InternalPayloadFactoryTest, InternalPayloadFromFileMessageWithoutIdReturnsNullptr) { PayloadTransferFrame frame; - std::string path = "C:\\Downloads"; + std::string path = "/tmp/Downloads"; frame.set_packet_type(PayloadTransferFrame::DATA); auto& header = *frame.mutable_payload_header(); header.set_type(PayloadTransferFrame::PayloadHeader::FILE); @@ -166,7 +166,7 @@ TEST(InternalPayloadFactoryTest, TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromFileMessageWithFileNameNotSet) { PayloadTransferFrame frame; - std::string path = "C:\\Downloads"; + std::string path = "/tmp/Downloads"; frame.set_packet_type(PayloadTransferFrame::DATA); auto& header = *frame.mutable_payload_header(); header.set_type(PayloadTransferFrame::PayloadHeader::FILE); @@ -180,10 +180,11 @@ TEST(InternalPayloadFactoryTest, Payload payload = internal_payload->ReleasePayload(); EXPECT_EQ(payload.GetFileName(), "12345"); } + TEST(InternalPayloadFactoryTest, CanCreateInternalPayloadFromFileMessageWithFileNameSet) { PayloadTransferFrame frame; - std::string path = "C:\\Downloads"; + std::string path = "/tmp/Downloads"; frame.set_packet_type(PayloadTransferFrame::DATA); auto& header = *frame.mutable_payload_header(); header.set_type(PayloadTransferFrame::PayloadHeader::FILE); @@ -200,6 +201,23 @@ TEST(InternalPayloadFactoryTest, EXPECT_EQ(payload.GetFileName(), "test.file.name"); } +TEST(InternalPayloadFactoryTest, + CreateInternalPayloadFailsIfFileCannotBeCreated) { + PayloadTransferFrame frame; + // /dev/null is a special file, no sub directories can be created + std::string path = "/dev/null"; + frame.set_packet_type(PayloadTransferFrame::DATA); + auto& header = *frame.mutable_payload_header(); + header.set_type(PayloadTransferFrame::PayloadHeader::FILE); + header.set_id(12345); + header.set_total_size(512); + header.set_file_name("test.file.name"); + header.set_parent_folder("Downloads2"); + ErrorOr> result = + CreateIncomingInternalPayload(frame, path); + ASSERT_TRUE(result.has_error()); +} + void CreateFileWithContents(Payload::Id payload_id, const ByteArray& contents) { OutputFile file(payload_id); EXPECT_TRUE(file.Write(contents).Ok()); diff --git a/internal/platform/file.cc b/internal/platform/file.cc index 5bb258a4..12fe4c1d 100644 --- a/internal/platform/file.cc +++ b/internal/platform/file.cc @@ -61,6 +61,7 @@ OutputFile::~OutputFile() = default; OutputFile::OutputFile(OutputFile&&) noexcept = default; OutputFile& OutputFile::operator=(OutputFile&&) = default; +bool OutputFile::IsValid() const { return impl_ != nullptr; } // Writes all data from ByteArray object to the underlying stream. // Returns Exception::kIo on error, Exception::kSuccess otherwise. Exception OutputFile::Write(const ByteArray& data) { diff --git a/internal/platform/file.h b/internal/platform/file.h index d72f9d8d..f88cab1c 100644 --- a/internal/platform/file.h +++ b/internal/platform/file.h @@ -77,6 +77,8 @@ class OutputFile final { OutputFile(OutputFile&&) noexcept; OutputFile& operator=(OutputFile&&); + bool IsValid() const; + // Writes all data from ByteArray object to the underlying stream. // Returns Exception::kIo on error, Exception::kSuccess otherwise. Exception Write(const ByteArray& data); diff --git a/internal/platform/implementation/g3/BUILD b/internal/platform/implementation/g3/BUILD index 7b79ff21..c79cf420 100644 --- a/internal/platform/implementation/g3/BUILD +++ b/internal/platform/implementation/g3/BUILD @@ -155,7 +155,9 @@ cc_library( ":comm", ":crypto", # build_cleaner: keep ":types", + "//internal/base:files", "//internal/platform:base", + "//internal/platform:logging", "//internal/platform:test_util", "//internal/platform/implementation:comm", "//internal/platform/implementation:platform", @@ -163,11 +165,9 @@ cc_library( "//internal/platform/implementation/shared:count_down_latch", "//internal/platform/implementation/shared:file", "@com_google_absl//absl/base:core_headers", - "@com_google_absl//absl/memory", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", - "@com_google_absl//absl/time", "@com_google_nisaba//nisaba/port:thread_pool", ], ) diff --git a/internal/platform/implementation/g3/platform.cc b/internal/platform/implementation/g3/platform.cc index a2b81be0..72e60604 100644 --- a/internal/platform/implementation/g3/platform.cc +++ b/internal/platform/implementation/g3/platform.cc @@ -14,19 +14,18 @@ #include "internal/platform/implementation/platform.h" -#include #include #include +#include // NOLINT #include #include #include "absl/base/attributes.h" -#include "absl/memory/memory.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" -#include "absl/time/time.h" +#include "internal/base/files.h" #include "internal/platform/implementation/atomic_boolean.h" #include "internal/platform/implementation/atomic_reference.h" #include "internal/platform/implementation/ble.h" @@ -51,6 +50,7 @@ #include "internal/platform/implementation/wifi_direct.h" #include "internal/platform/implementation/wifi_hotspot.h" #include "internal/platform/implementation/wifi_lan.h" +#include "internal/platform/logging.h" #include "internal/platform/os_name.h" #include "internal/platform/payload_id.h" #include "thread/thread.h" @@ -86,7 +86,7 @@ namespace api { std::string ImplementationPlatform::GetCustomSavePath( const std::string& parent_folder, const std::string& file_name) { - return absl::StrCat(parent_folder, file_name); + return absl::StrCat(parent_folder, "/", file_name); } std::string ImplementationPlatform::GetDownloadPath( @@ -162,6 +162,15 @@ std::unique_ptr ImplementationPlatform::CreateOutputFile( std::unique_ptr ImplementationPlatform::CreateOutputFile( const std::string& file_path) { + std::filesystem::path path = std::filesystem::u8path(file_path); + std::filesystem::path folder_path = path.parent_path(); + // Verifies that a path is a valid directory. + if (!sharing::DirectoryExists(folder_path)) { + if (!sharing::CreateDirectories(folder_path)) { + LOG(ERROR) << "Failed to create directory: " << folder_path.string(); + return nullptr; + } + } return shared::IOFile::CreateOutputFile(file_path); }