MTP: Add file transfer tests and fix libmtp not returning correct storage_id values in the object dict

src/calibre/devices/mtp/filesystem_cache.py

@@ -17,23 +17,30 @@ from calibre.utils.icu import sort_key, lower
 
 class FileOrFolder(object):
 
-    def __init__(self, entry, fs_cache, all_storage_ids=()):
+    def __init__(self, entry, fs_cache):
         self.object_id = entry['id']
         self.is_folder = entry['is_folder']
-        self.name = force_unicode(entry.get('name', '___'), 'utf-8')
+        n = entry.get('name', None)
+        if not n: n = '___'
+        self.name = force_unicode(n, 'utf-8')
         self.storage_id = entry.get('storage_id', None)
         self.persistent_id = entry.get('persistent_id', self.object_id)
         self.size = entry.get('size', 0)
         # self.parent_id is None for storage objects
         self.parent_id = entry.get('parent_id', None)
-        if self.parent_id == 0:
+        self.all_storage_ids = fs_cache.all_storage_ids
-            sid = self.storage_id
+
-            if all_storage_ids and sid not in all_storage_ids:
-                sid = all_storage_ids[0]
-            self.parent_id = sid
         if self.parent_id is None and self.storage_id is None:
             # A storage object
             self.storage_id = self.object_id
+
+        if self.storage_id not in self.all_storage_ids:
+            raise ValueError('Storage id %s not valid for %s'%(self.storage_id,
+                entry))
+
+        if self.parent_id == 0:
+            self.parent_id = self.storage_id
+
         self.is_hidden = entry.get('is_hidden', False)
         self.is_system = entry.get('is_system', False)
         self.can_delete = entry.get('can_delete', True)
@@ -42,6 +49,7 @@ class FileOrFolder(object):
         self.folders = []
         fs_cache.id_map[self.object_id] = self
         self.fs_cache = weakref.ref(fs_cache)
+        self.deleted = False
 
     @property
     def id_map(self):
@@ -80,6 +88,7 @@ class FileOrFolder(object):
             except ValueError:
                 pass
         self.id_map.pop(entry.object_id, None)
+        entry.deleted = True
 
     def dump(self, prefix='', out=sys.stdout):
         c = '+' if self.is_folder else '-'
@@ -110,16 +119,18 @@ class FilesystemCache(object):
     def __init__(self, all_storage, entries):
         self.entries = []
         self.id_map = {}
+        self.all_storage_ids = tuple(x['id'] for x in all_storage)
 
         for storage in all_storage:
-            e = FileOrFolder(storage, self, [])
+            e = FileOrFolder(storage, self)
             self.entries.append(e)
 
         self.entries.sort(key=attrgetter('object_id'))
         all_storage_ids = [x.object_id for x in self.entries]
+        self.all_storage_ids = tuple(all_storage_ids)
 
         for entry in entries:
-            FileOrFolder(entry, self, all_storage_ids)
+            FileOrFolder(entry, self)
 
         for item in self.id_map.itervalues():
             try:

src/calibre/devices/mtp/test.py

@@ -7,35 +7,49 @@ __license__   = 'GPL v3'
 __copyright__ = '2012, Kovid Goyal <kovid at kovidgoyal.net>'
 __docformat__ = 'restructuredtext en'
 
-import unittest, gc
+import unittest, gc, io
 
 from calibre.constants import iswindows, islinux
 from calibre.utils.icu import lower
 from calibre.devices.mtp.driver import MTP_DEVICE
 from calibre.devices.scanner import DeviceScanner
 
+class ProgressCallback(object):
+
+    def __init__(self):
+        self.count = 0
+        self.end_called = False
+
+    def __call__(self, pos, total):
+        if pos == total:
+            self.end_called = True
+        self.count += 1
+
 class TestDeviceInteraction(unittest.TestCase):
 
     @classmethod
     def setUpClass(cls):
+        cls.dev = cls.storage = None
         cls.dev = MTP_DEVICE(None)
         cls.dev.startup()
         cls.scanner = DeviceScanner()
         cls.scanner.scan()
         cd = cls.dev.detect_managed_devices(cls.scanner.devices)
         if cd is None:
-            raise ValueError('No MTP device found')
+            cls.dev.shutdown()
+            cls.dev = None
+            return
         cls.dev.open(cd, 'test_library')
         if cls.dev.free_space()[0] < 10*(1024**2):
-            raise ValueError('The connected device %s does not have enough free'
+            return
-                    ' space in its main memory to do the tests'%cd)
         cls.dev.filesystem_cache
         cls.storage = cls.dev.filesystem_cache.entries[0]
 
     @classmethod
     def tearDownClass(cls):
-        cls.dev.shutdown()
+        if cls.dev is not None:
-        cls.dev = None
+            cls.dev.shutdown()
+            cls.dev = None
 
     def setUp(self):
         self.cleanup = []
@@ -44,8 +58,15 @@ class TestDeviceInteraction(unittest.TestCase):
         for obj in reversed(self.cleanup):
             self.dev.delete_file_or_folder(obj)
 
+    def check_setup(self):
+        if self.dev is None:
+            self.skipTest('No MTP device detected')
+        if self.storage is None:
+            self.skipTest('The connected device does not have enough free space')
+
     def test_folder_operations(self):
         ''' Test the creation of folders, duplicate folders and sub folders '''
+        self.check_setup()
 
         # Create a folder
         name = 'zzz-test-folder'
@@ -78,6 +99,72 @@ class TestDeviceInteraction(unittest.TestCase):
             with self.assertRaises(ValueError):
                 self.dev.create_folder(root_file[0], 'sub-folder')
 
+    def test_file_transfer(self):
+        ''' Test transferring files to and from the device '''
+        self.check_setup()
+        # Create a folder
+        name = 'zzz-test-folder'
+        folder = self.dev.create_folder(self.storage, name)
+        self.cleanup.append(folder)
+        self.assertTrue(folder.is_folder)
+        self.assertEqual(folder.parent_id, self.storage.object_id)
+
+        # Check simple file put/get
+        size = 1024**2
+        raw = io.BytesIO(b'a'*size)
+        raw.seek(0)
+        name = 'test-file.txt'
+        pc = ProgressCallback()
+        f = self.dev.put_file(folder, name, raw, size, callback=pc)
+        self.cleanup.append(f)
+        self.assertEqual(f.name, name)
+        self.assertEqual(f.size, size)
+        self.assertEqual(f.parent_id, folder.object_id)
+        self.assertEqual(f.storage_id, folder.storage_id)
+        self.assertTrue(pc.end_called,
+                msg='Progress callback not called with equal values (put_file)')
+        self.assertTrue(pc.count > 1,
+                msg='Progress callback only called once (put_file)')
+
+        raw2 = io.BytesIO()
+        pc = ProgressCallback()
+        self.dev.get_file(f, raw2, callback=pc)
+        self.assertEqual(raw.getvalue(), raw2.getvalue())
+        self.assertTrue(pc.end_called,
+                msg='Progress callback not called with equal values (get_file)')
+        self.assertTrue(pc.count > 1,
+                msg='Progress callback only called once (get_file)')
+
+        # Check file replacement
+        raw = io.BytesIO(b'abcd')
+        raw.seek(0)
+        size = 4
+        f = self.dev.put_file(folder, name, raw, size)
+        self.cleanup.append(f)
+        self.assertEqual(f.name, name)
+        self.assertEqual(f.size, size)
+        self.assertEqual(f.parent_id, folder.object_id)
+        self.assertEqual(f.storage_id, folder.storage_id)
+
+        # Check that we get an error with replace=False
+        raw.seek(0)
+        with self.assertRaises(ValueError):
+            self.dev.put_file(folder, name, raw, size, replace=False)
+
+        # Check that we can put a file into the root
+        raw.seek(0)
+        name = 'zzz-test-file.txt'
+        f = self.dev.put_file(self.storage, name, raw, size)
+        self.cleanup.append(f)
+        self.assertEqual(f.name, name)
+        self.assertEqual(f.size, size)
+        self.assertEqual(f.parent_id, self.storage.object_id)
+        self.assertEqual(f.storage_id, self.storage.storage_id)
+
+        raw2 = io.BytesIO()
+        self.dev.get_file(f, raw2)
+        self.assertEqual(raw.getvalue(), raw2.getvalue())
+
     def measure_memory_usage(self, repetitions, func, *args, **kwargs):
         from calibre.utils.mem import memory
         gc.disable()
@@ -91,10 +178,10 @@ class TestDeviceInteraction(unittest.TestCase):
             gc.enable()
         return end_mem - start_mem
 
+    @unittest.skipUnless(iswindows or islinux, 'Can only test for leaks on windows and linux')
     def test_memory_leaks(self):
         ''' Test for memory leaks in the C modules '''
-        if not (iswindows or islinux):
+        self.check_setup()
-            self.skipTest('Can only test for leaks on windows and linux')
 
         # Test device scanning
         used_by_one = self.measure_memory_usage(1,
@@ -122,7 +209,7 @@ class TestDeviceInteraction(unittest.TestCase):
 
 def tests():
     tl = unittest.TestLoader()
-    return tl.loadTestsFromName('test.TestDeviceInteraction.test_memory_leaks')
+    # return tl.loadTestsFromName('test.TestDeviceInteraction.test_file_transfer')
     return tl.loadTestsFromTestCase(TestDeviceInteraction)
 
 def run():

src/calibre/devices/mtp/unix/driver.py

@@ -238,11 +238,44 @@ class MTP_DEVICE(MTPDeviceBase):
             raise DeviceError(
                     'Failed to create folder named %s in %s with error: %s'%
                     (name, parent.full_path, self.format_errorstack(errs)))
-        ans['storage_id'] = sid
         return parent.add_child(ans)
 
+    @synchronous
+    def put_file(self, parent, name, stream, size, callback=None, replace=True):
+        e = parent.folder_named(name)
+        if e is not None:
+            raise ValueError('Cannot upload file, %s already has a folder named: %s'%(
+                parent.full_path, e.name))
+        e = parent.file_named(name)
+        if e is not None:
+            if not replace:
+                raise ValueError('Cannot upload file %s, it already exists'%(
+                    e.full_path,))
+            self.delete_file_or_folder(e)
+        ename = name.encode('utf-8') if isinstance(name, unicode) else name
+        sid, pid = parent.storage_id, parent.object_id
+        if pid == sid:
+            pid = 0
+
+        ans, errs = self.dev.put_file(sid, pid, ename, stream, size, callback)
+        if ans is None:
+            raise DeviceError('Failed to upload file named: %s to %s: %s'
+                    %(name, parent.full_path, self.format_errorstack(errs)))
+        return parent.add_child(ans)
+
+    @synchronous
+    def get_file(self, f, stream, callback=None):
+        if f.is_folder:
+            raise ValueError('%s if a folder'%(f.full_path,))
+        ok, errs = self.dev.get_file(f.object_id, stream, callback)
+        if not ok:
+            raise DeviceError('Failed to get file: %s with errors: %s'%(
+                f.full_path, self.format_errorstack(errs)))
+
     @synchronous
     def delete_file_or_folder(self, obj):
+        if obj.deleted:
+            return
         if not obj.can_delete:
             raise ValueError('Cannot delete %s as deletion not allowed'%
                     (obj.full_path,))
@@ -255,7 +288,7 @@ class MTP_DEVICE(MTPDeviceBase):
         parent = obj.parent
         ok, errs = self.dev.delete_object(obj.object_id)
         if not ok:
-            raise DeviceError('Failed to delete %s with error: '%
+            raise DeviceError('Failed to delete %s with error: %s'%
                 (obj.full_path, self.format_errorstack(errs)))
         parent.remove_child(obj)
 

src/calibre/devices/mtp/unix/libmtp.c

@@ -67,7 +67,7 @@ static void dump_errorstack(LIBMTP_mtpdevice_t *dev, PyObject *list) {
     PyObject *err;
 
     for(stack = LIBMTP_Get_Errorstack(dev); stack != NULL; stack=stack->next) {
-        err = Py_BuildValue("Is", stack->errornumber, stack->error_text);
+        err = Py_BuildValue("is", stack->errornumber, stack->error_text);
         if (err == NULL) break;
         PyList_Append(list, err);
         Py_DECREF(err);
@@ -119,17 +119,43 @@ static uint16_t data_from_python(void *params, void *priv, uint32_t wantlen, uns
 }
 
 static PyObject* build_file_metadata(LIBMTP_file_t *nf, uint32_t storage_id) {
-    char *filename = nf->filename;
+    PyObject *ans = NULL, *l = NULL;
-    if (filename == NULL) filename = "";
 
-    return Py_BuildValue("{s:k,s:k,s:k,s:s,s:K,s:O}",
+    ans = Py_BuildValue("{s:s}", "name", nf->filename);
-                "id", nf->item_id,
+    if (ans == NULL) return PyErr_NoMemory();
-                "parent_id", nf->parent_id,
+
-                "storage_id", storage_id,
+    // We explicitly populate the dictionary instead of using Py_BuildValue to
-                "name", filename,
+    // handle the numeric variables properly. Without this, for some reason the
-                "size", nf->filesize,
+    // dict sometimes has incorrect values
-                "is_folder", (nf->filetype == LIBMTP_FILETYPE_FOLDER) ? Py_True : Py_False
+    l = PyLong_FromUnsignedLong(nf->item_id);
-    );
+    if (l == NULL) goto error;
+    if (PyDict_SetItemString(ans, "id", l) != 0) goto error;
+    Py_DECREF(l); l = NULL;
+
+    l = PyLong_FromUnsignedLong(nf->parent_id);
+    if (l == NULL) goto error;
+    if (PyDict_SetItemString(ans, "parent_id", l) != 0) goto error;
+    Py_DECREF(l); l = NULL;
+
+    l = PyLong_FromUnsignedLong(storage_id);
+    if (l == NULL) goto error;
+    if (PyDict_SetItemString(ans, "storage_id", l) != 0) goto error;
+    Py_DECREF(l); l = NULL;
+
+    l = PyLong_FromUnsignedLongLong(nf->filesize);
+    if (l == NULL) goto error;
+    if (PyDict_SetItemString(ans, "size", l) != 0) goto error;
+    Py_DECREF(l); l = NULL;
+
+    if (PyDict_SetItemString(ans, "is_folder",
+        (nf->filetype == LIBMTP_FILETYPE_FOLDER) ? Py_True : Py_False) != 0) 
+        goto error;
+
+    return ans;
+
+error:
+    Py_XDECREF(ans); Py_XDECREF(l);
+    return PyErr_NoMemory();
 }
 
 static PyObject* file_metadata(LIBMTP_mtpdevice_t *device, PyObject *errs, uint32_t item_id, uint32_t storage_id) {
@@ -507,9 +533,8 @@ libmtp_Device_delete_object(libmtp_Device *self, PyObject *args, PyObject *kwarg
 static PyObject *
 libmtp_Device_create_folder(libmtp_Device *self, PyObject *args, PyObject *kwargs) {
     PyObject *errs, *fo = NULL;
-    uint32_t parent_id, storage_id;
+    uint32_t storage_id, parent_id, folder_id;
     char *name;
-    uint32_t folder_id;
 
     ENSURE_DEV(NULL); ENSURE_STORAGE(NULL);
 

src/calibre/devices/mtp/windows/content_enumeration.cpp

@@ -73,7 +73,7 @@ static void set_size_property(PyObject *dict, REFPROPERTYKEY key, const char *py
     hr = properties->GetUnsignedLargeIntegerValue(key, &val);
 
     if (SUCCEEDED(hr)) {
-        pval = PyInt_FromSsize_t((Py_ssize_t)val);
+        pval = PyLong_FromUnsignedLongLong(val);
         if (pval != NULL) {
             PyDict_SetItemString(dict, pykey, pval);
             Py_DECREF(pval);

src/calibre/devices/mtp/windows/driver.py

@@ -275,6 +275,8 @@ class MTP_DEVICE(MTPDeviceBase):
 
     @same_thread
     def delete_file_or_folder(self, obj):
+        if obj.deleted:
+            return
         if not obj.can_delete:
             raise ValueError('Cannot delete %s as deletion not allowed'%
                     (obj.full_path,))