You're right about bug report, but it can fail in any other place, just in particular version it happen in updateStorageInfo Can we cache getDevice in m_device (in constructor) then use it everywhere. I think libmtp has guard against disconnected device and will not crash.

In D28535#640656, @anthonyfieroni wrote:

You're right about bug report, but it can fail in any other place, just in particular version it happen in updateStorageInfo Can we cache getDevice in m_device (in constructor) then use it everywhere. I think libmtp has guard against disconnected device and will not crash.

MTPDevice already does that.

If getDevice() returns nullptr, this means that MTPDevice::getDevice() returns nullptr. This can only happen if m_mtpdevice is nullptr, which will crash in MTPDevice::~MTPDevice sooner or later anyway.

So this patch will at most just delay the crash.

AFAICT MTPDevice is supposed to be destroyed in KMTPd::deviceRemoved on disconnection, but this is obviously racy.

In D28535#640672, @fvogt wrote:

If getDevice() returns nullptr, this means that MTPDevice::getDevice() returns nullptr. This can only happen if m_mtpdevice is nullptr, which will crash in MTPDevice::~MTPDevice sooner or later anyway.

I don't think so, libmtp knows device is not available then LIBMTP_Release_Device (invalid device) will not crash. So i prefer to cache device in storage as well so libmtp will just return false on invalid device.

There is no such thing as an "invalid device" at that point anymore. There's only nullptr.

LIBMTP_mtpdevice_t *MTPDevice::getDevice()
{
    if (!m_mtpdevice->storage) {
        qCDebug(LOG_KIOD_KMTPD) << "no storage found: reopen mtpdevice";
        LIBMTP_Release_Device(m_mtpdevice);
        m_mtpdevice = LIBMTP_Open_Raw_Device_Uncached(&m_rawdevice);
    }

    return m_mtpdevice;
}

What you're suggesting is to change MTPDevice::getDevice to return the old device if reopening fails - but reopening without releasing might not work.

I see we don't speak in same language :)
LIBMTP_Open_Raw_Device_Uncached(&m_rawdevice);
returns nullptr that's normal since device is inaccessible, i mean it does not need to call LIBMTP_Release_Device using m_mtpdevice is safe it's not nullptr, it's just a disconnected device and libmtp knows that.

In D28535#640680, @anthonyfieroni wrote:

I see we don't speak in same language :)
LIBMTP_Open_Raw_Device_Uncached(&m_rawdevice);
returns nullptr that's normal since device is inaccessible, i mean it does not need to call LIBMTP_Release_Device using m_mtpdevice is safe it's not nullptr, it's just a disconnected device and libmtp knows that.

Yes, and until that point everything is fine.
Only after m_mtpdevice = LIBMTP_Open_Raw_Device_Uncached(&m_rawdevice);, which sets m_mtpdevice to nullptr it goes down the path I outlined.

In D28535#640674, @fvogt wrote:

What you're suggesting is to change MTPDevice::getDevice to return the old device if reopening fails - but reopening without releasing might not work.

This seems to be a robust solution IMO, why do you suspect this might not work?

In D28535#640682, @feverfew wrote:

In D28535#640674, @fvogt wrote:

What you're suggesting is to change MTPDevice::getDevice to return the old device if reopening fails - but reopening without releasing might not work.

This seems to be a robust solution IMO, why do you suspect this might not work?

Because there can only be a single libusb session per device. So you have to release the old one before opening again.

@feverfew are you gonna try what i'm writing about or i should do it? Just use cached device, do not reopen since it'll return nullptr.

If you want to, feel free, I'm a bit tight on time. But I will say this. the whole getDevice() function confuses me. I'm not entirely sure why this if check is necessary at all. The lifetime of devices and its children (i.e storage) should be managed by the KMTPD daemon AFAICT. A device shouldn't be trying to re-open itself anywhere IMO. To me the getDevice() function should simply be a simple return to avoid this NULL issue happening. Even if a device doesn't exist anymore, no segfaults should happen when passing an "invalid" LIBMTP_mtpdevice_t to any other LIBMTP function?

So to be succinct, the only correct fix here is to change getDevice() to return m_mtpdevice?

In D28535#640699, @feverfew wrote:

So to be succinct, the only correct fix here is to change getDevice() to return m_mtpdevice?

Yes, then check if it's crash, in all other places LIBMTP_xxx should take care of and return false or nullptr depend of function returning value.

In D28535#640703, @anthonyfieroni wrote:

In D28535#640699, @feverfew wrote:

So to be succinct, the only correct fix here is to change getDevice() to return m_mtpdevice?

Yes, then check if it's crash, in all other places LIBMTP_xxx should take care of and return false or nullptr depend of function returning value.

What do you mean by "then check if it's crash"? Surely we should do nothing and let LIBMTP sort out printing errors and stuff? Eventually the device should be removed by the daemon anyway via the help of Solid::deviceRemoved(), and hence LIBMTP_Release_Device will be called once and in the correct place. I agree with everything after the comma though, LIBMTP_mtpdevice_t * should never be freed unless we explicitly do so ourselves.

I assume there is a reason why MTPDevice::getDevice() has code for handling this very specific case, so I wouldn't just remove it without figuring out why: https://i.redd.it/hfnl7xo8yovy.gif

If not, that would indeed be the best option.

Ok so I've updated the code as can be seen. Mucked around with disconnecting/connecting, no issues on my side at least.

Ok that's look good to me. We can move that code in constructor just before loop for storages, then use m_mtpdevice instead of raw device, but it looks like removed code is just a noise and it shouldn't present at all.

+1

@elvisangelaccio this peace of code is purely wrong at least m_storages is not updated to new device and not only. This code should never exists or try to hide some other bug.

In D28535#642289, @elvisangelaccio wrote:

In D28535#640833, @fvogt wrote:

I assume there is a reason why MTPDevice::getDevice() has code for handling this very specific case, so I wouldn't just remove it without figuring out why: https://i.redd.it/hfnl7xo8yovy.gif

If not, that would indeed be the best option.

Unfortunately git blame doesn't seem to help us here.

I suggest to push this fix to master only and see what happens.

By pushing this to master would we still be able to throw it up to 20.04.* if we decide it's stable enough? (also need to know to know what to put down as the FIXED-IN in the commit message)?

In D28535#642298, @feverfew wrote:

In D28535#642289, @elvisangelaccio wrote:

In D28535#640833, @fvogt wrote:

I assume there is a reason why MTPDevice::getDevice() has code for handling this very specific case, so I wouldn't just remove it without figuring out why: https://i.redd.it/hfnl7xo8yovy.gif

If not, that would indeed be the best option.

Unfortunately git blame doesn't seem to help us here.

I suggest to push this fix to master only and see what happens.

By pushing this to master would we still be able to throw it up to 20.04.* if we decide it's stable enough? (also need to know to know what to put down as the FIXED-IN in the commit message)?

@elvisangelaccio ping seeming as 20.04 release is close...

In D28535#642298, @feverfew wrote:

In D28535#642289, @elvisangelaccio wrote:

In D28535#640833, @fvogt wrote:

I assume there is a reason why MTPDevice::getDevice() has code for handling this very specific case, so I wouldn't just remove it without figuring out why: https://i.redd.it/hfnl7xo8yovy.gif

If not, that would indeed be the best option.

Unfortunately git blame doesn't seem to help us here.

I suggest to push this fix to master only and see what happens.

By pushing this to master would we still be able to throw it up to 20.04.* if we decide it's stable enough? (also need to know to know what to put down as the FIXED-IN in the commit message)?

Sure, we can cherry-pick that commit later.