Fix error handling in AdbClient - PushFile and PullFile

[ovyalov]

Summary

PushFile - fixed DONE response handling.
PullFile - fixed RECV response handling.
Added STAT request.

[labath]

source/Plugins/Platform/Android/AdbClient.cpp

272

This will fail if the file exists but is of zero size. In any case, I don't think the solution is to do a stat() before we attempt the transfer, since there are lots of ways the transfer can go south even if the stat call succeeds (file has wrong permissions, file has been removed in the mean time, uncorrectable read error, connection got terminated, ...). Adb already sends us the exact error that occurred in response to the RECV command, but we fail to parse it out for some reason. For example, if I remove the read permission on the file, adb sends the 'Permission denied' error, but for some reason we end up displaying:

get-file failed: Failed to read file chunk: Got unexpected response id from adb: "issi"

The "issi" string is actually the substring of the "Permission denied", so it looks there there is some error in the protocol parsing. Isn't it possible to do something similar as in the PushFile implementation?

[ovyalov]

source/Plugins/Platform/Android/AdbClient.cpp

272

I traced STAT command used by adb pull by missed that RECV command may have FAIL response with error message - fixed that.
Left Stat as-is since it's already implemented and if somebody will need it in future.

[ovyalov]

Addressed review comments.

[labath]

LGTM, thanks.

[ovyalov]

AFFECTED FILES

/lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp
/lldb/trunk/source/Plugins/Platform/Android/AdbClient.h

USERS

ovyalov (Author)

http://reviews.llvm.org/rL239130

source/Plugins/Platform/Android/AdbClient.h

public:
     DeletePortForwarding (const uint16_t port);
 
     Error
     PullFile (const FileSpec &remote_file, const FileSpec &local_file);
 
     Error
     PushFile (const FileSpec &local_file, const FileSpec &remote_file);
 
+    Error
+    Stat (const FileSpec &remote_file, uint32_t &mode, uint32_t &size, uint32_t &mtime);
+
 private:
     Error
     Connect ();
 
     void
     SetDeviceID (const std::string &device_id);
 
     Error

source/Plugins/Platform/Android/AdbClient.cpp

 using namespace lldb_private;
 using namespace lldb_private::platform_android;
 
 namespace {
 
 const uint32_t kReadTimeout = 1000000; // 1 second
 const char * kOKAY = "OKAY";
 const char * kFAIL = "FAIL";
+const char * kDATA = "DATA";
+const char * kDONE = "DONE";
+
+const char * kSEND = "SEND";
+const char * kRECV = "RECV";
+const char * kSTAT = "STAT";
+
 const size_t kSyncPacketLen = 8;
 // Maximum size of a filesync DATA packet.
 const size_t kMaxPushData = 2*1024;
 // Default mode for pushed files.
 const uint32_t kDefaultMode = 0100770; // S_IFREG | S_IRWXU | S_IRWXG
 
 }  // namespace
 
if (error.Fail ())
         return error;
 
     const auto local_file_path = local_file.GetPath ();
     llvm::FileRemover local_file_remover (local_file_path.c_str ());
 
     std::ofstream dst (local_file_path, std::ios::out | std::ios::binary);
     if (!dst.is_open ())
         return Error ("Unable to open local file %s", local_file_path.c_str());
 
     const auto remote_file_path = remote_file.GetPath (false);
-    error = SendSyncRequest ("RECV", remote_file_path.length (), remote_file_path.c_str ());
+    error = SendSyncRequest (kRECV, remote_file_path.length (), remote_file_path.c_str ());
     if (error.Fail ())
         return error;
 
     std::vector<char> chunk;
     bool eof = false;
     while (!eof)
     {
         error = PullFileChunk (chunk, eof);
         if (error.Fail ())
-            return Error ("Failed to read file chunk: %s", error.AsCString ());
+            return error;
         if (!eof)
             dst.write (&chunk[0], chunk.size ());
     }
 
     local_file_remover.releaseFile ();
     return error;
 }
 
 Error
 AdbClient::PushFile (const FileSpec &local_file, const FileSpec &remote_file)
 {
     auto error = StartSync ();
     if (error.Fail ())
         return error;
 
     const auto local_file_path (local_file.GetPath ());
     std::ifstream src (local_file_path.c_str(), std::ios::in | std::ios::binary);
     if (!src.is_open ())
         return Error ("Unable to open local file %s", local_file_path.c_str());
 
     std::stringstream file_description;
     file_description << remote_file.GetPath(false).c_str() << "," << kDefaultMode;
     std::string file_description_str = file_description.str();
-    error = SendSyncRequest ("SEND", file_description_str.length(), file_description_str.c_str());
+    error = SendSyncRequest (kSEND, file_description_str.length(), file_description_str.c_str());
     if (error.Fail ())
         return error;
 
     char chunk[kMaxPushData];
     while (!src.eof() && !src.read(chunk, kMaxPushData).bad())
     {
         size_t chunk_size = src.gcount();
-        error = SendSyncRequest("DATA", chunk_size, chunk);
+        error = SendSyncRequest(kDATA, chunk_size, chunk);
         if (error.Fail ())
             return Error ("Failed to send file chunk: %s", error.AsCString ());
     }
-    error = SendSyncRequest("DONE", local_file.GetModificationTime().seconds(), nullptr);
+    error = SendSyncRequest(kDONE, local_file.GetModificationTime().seconds(), nullptr);
     if (error.Fail ())
         return error;
-    error = ReadResponseStatus();
+
+    std::string response_id;
+    uint32_t data_len;
+    error = ReadSyncHeader (response_id, data_len);
+    if (error.Fail ())
+        return Error ("Failed to read DONE response: %s", error.AsCString ());
+    if (response_id == kFAIL)
+    {
+        std::string error_message (data_len, 0);
+        error = ReadAllBytes (&error_message[0], data_len);
+        if (error.Fail ())
+            return Error ("Failed to read DONE error message: %s", error.AsCString ());
+        return Error ("Failed to push file: %s", error_message.c_str ());
+    }
+    else if (response_id != kOKAY)
+        return Error ("Got unexpected DONE response: %s", response_id.c_str ());
+
     // If there was an error reading the source file, finish the adb file
     // transfer first so that adb isn't expecting any more data.
     if (src.bad())
         return Error ("Failed read on %s", local_file_path.c_str());
     return error;
 }
 
 Error
AdbClient::PullFileChunk (std::vector<char> &buffer, bool &eof)
     buffer.clear ();
 
     std::string response_id;
     uint32_t data_len;
     auto error = ReadSyncHeader (response_id, data_len);
     if (error.Fail ())
         return error;
 
-    if (response_id == "DATA")
+    if (response_id == kDATA)
     {
         buffer.resize (data_len, 0);
         error = ReadAllBytes (&buffer[0], data_len);
         if (error.Fail ())
             buffer.clear ();
     }
-    else if (response_id == "DONE")
+    else if (response_id == kDONE)
+    {
         eof = true;
+    }
+    else if (response_id == kFAIL)
+    {
+        std::string error_message (data_len, 0);
+        error = ReadAllBytes (&error_message[0], data_len);
+        if (error.Fail ())
+            return Error ("Failed to read pull error message: %s", error.AsCString ());
+        return Error ("Failed to pull file: %s", error_message.c_str ());
+    }
     else
-        error = GetResponseError (response_id.c_str ());
+        return Error ("Pull failed with unknown response: %s", response_id.c_str ());
 
-    return error;
+    return Error ();
 }
 
 Error
 AdbClient::SendSyncRequest (const char *request_id, const uint32_t data_len, const void *data)
 {
     const DataBufferSP data_sp (new DataBufferHeap (kSyncPacketLen, 0));
     DataEncoder encoder (data_sp, eByteOrderLittle, sizeof (void*));
     auto offset = encoder.PutData (0, request_id, strlen(request_id));
AdbClient::ReadAllBytes (void *buffer, size_t size)
     {
         auto read_bytes = m_conn.Read (read_buffer + tota_read_bytes, size - tota_read_bytes, kReadTimeout, status, &error);
         if (error.Fail ())
             return error;
         tota_read_bytes += read_bytes;
     }
     return error;
 }
+
+Error
+AdbClient::Stat (const FileSpec &remote_file, uint32_t &mode, uint32_t &size, uint32_t &mtime)
+{
+    auto error = StartSync ();
+    if (error.Fail ())
+        return error;
+
+    const std::string remote_file_path (remote_file.GetPath (false));
+    error = SendSyncRequest (kSTAT, remote_file_path.length (), remote_file_path.c_str ());
+    if (error.Fail ())
+        return Error ("Failed to send request: %s", error.AsCString ());
+
+    static const size_t stat_len = strlen (kSTAT);
+    static const size_t response_len = stat_len + (sizeof (uint32_t) * 3);
+
+    std::vector<char> buffer (response_len);
+    error = ReadAllBytes (&buffer[0], buffer.size ());
+    if (error.Fail ())
+        return Error ("Failed to read response: %s", error.AsCString ());
+
+    DataExtractor extractor (&buffer[0], buffer.size (), eByteOrderLittle, sizeof (void*));
+    offset_t offset = 0;
+
+    const void* command = extractor.GetData (&offset, stat_len);
+    if (!command)
+        return Error ("Failed to get response command");
+    const char* command_str = static_cast<const char*> (command);
+    if (strncmp (command_str, kSTAT, stat_len))
+        return Error ("Got invalid stat command: %s", command_str);
+
+    mode = extractor.GetU32 (&offset);
+    size = extractor.GetU32 (&offset);
+    mtime = extractor.GetU32 (&offset);
+    return Error ();
+}