D15277: [RFC] kio_mtp: Move MTP device handling from kioslave to kiod-module

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  This is excellent work, thanks a lot for doing this. I just have "a few" minor comments... ;)

INLINE COMMENTS

> kio_mtp.cpp:107
>  {
> -    qCDebug(LOG_KIO_MTP) << url.path();
> +    qCDebug(LOG_KIO_MTP) << Q_FUNC_INFO << url.path();
>  

Don't use Q_FUNC_INFO in code.
Instead, set up your environment with %{function} in QT_MESSAGE_PATTERN.
For instance, mine says

'%{time h:mm:ss.zzz} %{appname}(%{pid}) %{if-category}%{category}: %{endif}%{if-debug}%{function}%{endif}%{if-warning}%{backtrace depth=3}%{endif}%{if-critical}%{backtrace depth=3}%{endif}%{if-fatal}%{backtrace depth=3}%{endif} %{message}'

See https://woboq.com/blog/nice-debug-output-with-qt.html for more info.

> kio_mtp.cpp:139
>  
> -            getEntry(entry, device);
> -
> -            listEntry(entry);
> -            entry.clear();
> +        for (const KMTPDeviceInterface *device : m_kmtpDaemon.devices()) {
> +            listEntry(getEntry(device));

move m_kmtpDaemon.devices to a const local variable, to avoid a detach. Yes this is annoying.

> kio_mtp.cpp:320
> +        const KMTPDeviceInterface *mtpDevice = m_kmtpDaemon.deviceFromName(pathItems.first());
> +        if (mtpDevice) {
> +            KMTPStorageInterface *storage = mtpDevice->storageFromDescription(pathItems.at(1));

and if mtpDevice is nullptr? This code doesn't seem to emit either error or finished, which is a bug.

Or if it can't be null, use Q_ASSERT instead of if() ;)

(Same in most other SlaveBase methods, please check that they all end up with either error() or finished())

> kio_mtp.cpp:338
> +                });
> +                connect(storage, &KMTPStorageInterface::copyFinished, &loop, &QEventLoop::exit);
> +                const int result = loop.exec();

Let's hope this signal is always ALWAYS emitted, including in all error cases...

> kio_mtp.h:79
> +
> +    static UDSEntry getEntry(const KMTPDeviceInterface *device);
> +    static UDSEntry getEntry(const KMTPStorageInterface *storage);

class-static private functions pollute the header file for no benefit, they might as well become file-static functions (i.e. remove the declarations from here, and remove the classname from the implementation, and ensure they're at the top of the file).

> kmtpd.cpp:59
> +    // Release devices
> +    for (const MTPDevice *device : m_devices) {
> +        deviceRemoved(device->udi());

qAsConst(m_devices) to avoid a detach

> kmtpd.cpp:128
> +
> +MTPDevice *KMTPd::deviceFromUdi(const QString &udi)
> +{

this method could probably be const?

> kmtpd.cpp:153
> +    if (device.isDeviceInterface(Solid::DeviceInterface::PortableMediaPlayer)) {
> +        qCDebug(LOG_KIOD_KMTPD) << "SOLID: New Device with udi=" << udi << "||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||";
> +

please clean up the "||||" stuff

> kmtpd.cpp:163
> +    if (device) {
> +        qCDebug(LOG_KIOD_KMTPD) << "SOLID: Device with udi=" << udi << " removed. ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||";
> +

same

> kmtpd.cpp:165
> +
> +        m_devices.removeAt(m_devices.indexOf(device));
> +        delete device;

There's removeOne(device)

> mtpstorage.cpp:206
> +{
> +    QDateTime dateTime = QDateTime::currentDateTime();
> +    dateTime = dateTime.addSecs(timeToLive);

Any chance currentDateTimeUtc could be used everywhere? It's much faster than currentDateTime because it doesn't need to do timezone conversions (which use the awful process-global tzset()).

Just a thought, maybe you do need local times.

> mtpstorage.cpp:225
> +        path.append(QLatin1Char('/'));
> +        path.append(pathItems.at(i));
> +    }

This method could be made much faster by locating the Nth slash and then using left(), to avoid the many reallocations due to multiple appends. Not sure it matters though.

> mtpstorage.cpp:256
> +
> +    emit const_cast<MTPStorage *>(static_cast<const MTPStorage *>(priv))->copyProgress(sent, total);
> +    return LIBMTP_HANDLER_RETURN_OK;

Minor: this would be more readable with a MTPStorage * local variable, i.e. splitting this over two lines.

> mtpstorage.cpp:473
> +{
> +    qCDebug(LOG_KIOD_KMTPD) << Q_FUNC_INFO << path;
> +

Please remove Q_FUNC_INFO everywhere.

> mtpstorage.cpp:499
> +
> +        // big files take some time to copy, and this may lead into D-Bus timeouts.
> +        // therefore the actual copying is not done within the D-Bus method itself but right after we return to the event loop

DBus timeouts are actually configurable, if you need to block until the copy is done, btw.

> mtpstorage.h:116
> +     */
> +    static uint16_t onDataPut(void *, void *priv, uint32_t sendlen, unsigned char *data, uint32_t *putlen);
> +    static int onDataProgress(const uint64_t sent, const uint64_t total, const void * const priv);

you know what I think of private class-static methods ;-)

> kmtpstorageinterface.cpp:76
> +    argumentList << QVariant::fromValue(path);
> +    return callWithArgumentList(QDBus::Block, QStringLiteral("getFileToHandler"), argumentList);
> +}

As others said, this file should really be generated by qdbusxml2cpp instead.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D15277

To: akrutzler, elvisangelaccio, ltoscano, hetzenecker, dfaure
Cc: kde-frameworks-devel, kfm-devel, feverfew, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20180922/80dc45db/attachment.htm>