Skip to content
Open
11 changes: 11 additions & 0 deletions src/app/drivemanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,12 @@ void DriveManager::onDriveConnected(Drive *d)
endInsertRows();
emit drivesChanged();

// Fix for issue #825: Only change selection if the currently selected drive is not busy
if (selected() && selected()->isBusy()) {
return;
}

// Standard logic: Select the new drive if not busy
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this comment can be removed

if (m_provider->initialized()) {
m_selectedIndex = position;
emit selectedChanged();
Expand Down Expand Up @@ -289,6 +295,11 @@ bool Drive::delayedWrite() const
return m_delayedWrite;
}

bool Drive::isBusy() const
{
return m_process && m_process->state() != QProcess::NotRunning;
}

void Drive::setDelayedWrite(const bool &o)
{
if (m_delayedWrite != o) {
Expand Down
32 changes: 32 additions & 0 deletions src/app/drivemanager.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

#include <QAbstractListModel>
#include <QDebug>
#include <QProcess>
#include <memory>

#include "releasemanager.h"

Expand All @@ -30,6 +32,34 @@ class DriveProvider;
class Drive;
class UdisksDrive;

/**
* @brief Custom deleter for QProcess to ensure proper cleanup
*
* Handles platform-specific process termination before deletion
*/
struct DriveOperationDeleter {
void operator()(QProcess *process) {
if (!process) {
return;
}

if (process->state() != QProcess::NotRunning) {
#ifdef Q_OS_WIN
process->kill();
process->waitForFinished();
#else
process->terminate();
if (!process->waitForFinished(3000)) {
process->kill();
process->waitForFinished();
}
#endif
}

delete process;
}
};

/**
* @brief The DriveManager class
*
Expand Down Expand Up @@ -169,6 +199,7 @@ class Drive : public QObject
virtual qreal size() const;
virtual RestoreStatus restoreStatus();
virtual bool delayedWrite() const;
virtual bool isBusy() const;

virtual void setDelayedWrite(const bool &o);

Expand All @@ -194,6 +225,7 @@ public slots:
RestoreStatus m_restoreStatus{CLEAN};
QString m_error{};
bool m_delayedWrite{false};
std::unique_ptr<QProcess, DriveOperationDeleter> m_process;
};

#endif // DRIVEMANAGER_H
44 changes: 18 additions & 26 deletions src/app/linuxdrivemanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,8 @@ bool LinuxDrive::write(ReleaseVariant *data)
if (m_image->status() == ReleaseVariant::READY || m_image->status() == ReleaseVariant::FAILED || m_image->status() == ReleaseVariant::FAILED_VERIFICATION || m_image->status() == ReleaseVariant::FINISHED)
m_image->setStatus(ReleaseVariant::WRITING);

if (!m_process)
m_process = new QProcess(this);
// Create new process using unique_ptr
m_process.reset(new QProcess(this));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_process = std::make_unique<QProcess>(this);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also remove the comment.


QStringList args;
if (QFile::exists(qApp->applicationDirPath() + "/../helper/linux/helper")) {
Expand All @@ -235,11 +235,11 @@ bool LinuxDrive::write(ReleaseVariant *data)
mDebug() << this->metaObject()->className() << "Helper command will be" << args;
m_process->setArguments(args);

connect(m_process, &QProcess::readyRead, this, &LinuxDrive::onReadyRead);
connect(m_process, SIGNAL(finished(int, QProcess::ExitStatus)), this, SLOT(onFinished(int, QProcess::ExitStatus)));
connect(m_process.get(), &QProcess::readyRead, this, &LinuxDrive::onReadyRead);
connect(m_process.get(), SIGNAL(finished(int, QProcess::ExitStatus)), this, SLOT(onFinished(int, QProcess::ExitStatus)));
// TODO check if this is actually necessary - it should work just fine even without it
connect(m_process, &QProcess::errorOccurred, this, &LinuxDrive::onErrorOccurred);
connect(qApp, &QCoreApplication::aboutToQuit, m_process, &QProcess::terminate);
connect(m_process.get(), &QProcess::errorOccurred, this, &LinuxDrive::onErrorOccurred);
connect(qApp, &QCoreApplication::aboutToQuit, m_process.get(), &QProcess::terminate);

m_process->start(QIODevice::ReadOnly);

Expand All @@ -250,7 +250,7 @@ void LinuxDrive::cancel()
{
Drive::cancel();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why only the LinuxDrive does something extra in the reimplementation of cancel() but I think we can remove it altogether so none of the cancel() reimplementations exists. It wouldn't work this way anyway, because calling Drive::cancel() first it clears the m_process so the condition that is checked below will never be satisfied.

static bool beingCancelled = false;
if (m_process != nullptr && !beingCancelled) {
if (m_process && !beingCancelled) {
beingCancelled = true;
if (m_image) {
if (m_image->status() == ReleaseVariant::WRITE_VERIFYING) {
Expand All @@ -260,9 +260,8 @@ void LinuxDrive::cancel()
m_image->setStatus(ReleaseVariant::FAILED);
}
}
m_process->kill();
m_process->deleteLater();
m_process = nullptr;
// Reset the unique_ptr, which automatically invokes DriveOperationDeleter
m_process.reset();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed in favor of generic implementation in Drive::cancel().

beingCancelled = false;
}
}
Expand All @@ -271,8 +270,8 @@ void LinuxDrive::restore()
{
mDebug() << this->metaObject()->className() << "Will now restore" << this->m_device;

if (!m_process)
m_process = new QProcess(this);
// Create new process using unique_ptr
m_process.reset(new QProcess(this));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, use std::make_unique


m_restoreStatus = RESTORING;
emit restoreStatusChanged();
Expand All @@ -294,9 +293,9 @@ void LinuxDrive::restore()
mDebug() << this->metaObject()->className() << "Helper command will be" << args;
m_process->setArguments(args);

connect(m_process, &QProcess::readyRead, this, &LinuxDrive::onReadyRead);
connect(m_process, SIGNAL(finished(int, QProcess::ExitStatus)), this, SLOT(onRestoreFinished(int, QProcess::ExitStatus)));
connect(qApp, &QCoreApplication::aboutToQuit, m_process, &QProcess::terminate);
connect(m_process.get(), &QProcess::readyRead, this, &LinuxDrive::onReadyRead);
connect(m_process.get(), SIGNAL(finished(int, QProcess::ExitStatus)), this, SLOT(onRestoreFinished(int, QProcess::ExitStatus)));
connect(qApp, &QCoreApplication::aboutToQuit, m_process.get(), &QProcess::terminate);

m_process->start(QIODevice::ReadOnly);
}
Expand Down Expand Up @@ -353,11 +352,8 @@ void LinuxDrive::onFinished(int exitCode, QProcess::ExitStatus status)
}
}

if (m_process) {
m_process->deleteLater();
m_process = nullptr;
m_image = nullptr;
}
// Process cleanup handled automatically by unique_ptr deleter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should also do m_process.reset().

m_image = nullptr;
}

void LinuxDrive::onRestoreFinished(int exitCode, QProcess::ExitStatus status)
Expand All @@ -373,10 +369,7 @@ void LinuxDrive::onRestoreFinished(int exitCode, QProcess::ExitStatus status)
} else {
m_restoreStatus = RESTORED;
}
if (m_process) {
m_process->deleteLater();
m_process = nullptr;
}
// Process cleanup handled automatically by unique_ptr deleter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should also do m_process.reset().

emit restoreStatusChanged();
}

Expand All @@ -389,8 +382,7 @@ void LinuxDrive::onErrorOccurred(QProcess::ProcessError e)
QString errorMessage = m_process->errorString();
mWarning() << "Restoring failed:" << errorMessage;
m_image->setErrorString(errorMessage);
m_process->deleteLater();
m_process = nullptr;
// Process cleanup handled automatically by unique_ptr deleter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should also do m_process.reset().

m_image->setStatus(ReleaseVariant::FAILED);
m_image = nullptr;
}
2 changes: 0 additions & 2 deletions src/app/linuxdrivemanager.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,6 @@ private slots:

private:
QString m_device;

QProcess *m_process{nullptr};
};

#endif // LINUXDRIVEMANAGER_H
68 changes: 30 additions & 38 deletions src/app/macdrivemanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,19 +87,16 @@ bool MacDrive::write(ReleaseVariant *data)
if (m_image->status() == ReleaseVariant::READY || m_image->status() == ReleaseVariant::FAILED || m_image->status() == ReleaseVariant::FAILED_VERIFICATION || m_image->status() == ReleaseVariant::FINISHED)
m_image->setStatus(ReleaseVariant::WRITING);

if (m_child) {
// TODO some handling of an already present process
m_child->deleteLater();
}
m_child = new QProcess(this);
connect(m_child, static_cast<void (QProcess::*)(int, QProcess::ExitStatus)>(&QProcess::finished), this, &MacDrive::onFinished);
connect(m_child, &QProcess::readyRead, this, &MacDrive::onReadyRead);
connect(qApp, &QCoreApplication::aboutToQuit, m_child, &QProcess::terminate);
// Create new process using unique_ptr
m_process.reset(new QProcess(this));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::make_unique and drop the comment

connect(m_process.get(), static_cast<void (QProcess::*)(int, QProcess::ExitStatus)>(&QProcess::finished), this, &MacDrive::onFinished);
connect(m_process.get(), &QProcess::readyRead, this, &MacDrive::onReadyRead);
connect(qApp, &QCoreApplication::aboutToQuit, m_process.get(), &QProcess::terminate);

if (QFile::exists(qApp->applicationDirPath() + "/../../../../helper/mac/helper.app/Contents/MacOS/helper")) {
m_child->setProgram(QString("%1/../../../../helper/mac/helper.app/Contents/MacOS/helper").arg(qApp->applicationDirPath()));
m_process->setProgram(QString("%1/../../../../helper/mac/helper.app/Contents/MacOS/helper").arg(qApp->applicationDirPath()));
} else if (QFile::exists(qApp->applicationDirPath() + "/helper")) {
m_child->setProgram(QString("%1/helper").arg(qApp->applicationDirPath()));
m_process->setProgram(QString("%1/helper").arg(qApp->applicationDirPath()));
} else {
data->setErrorString(tr("Could not find the helper binary. Check your installation."));
setDelayedWrite(false);
Expand All @@ -114,39 +111,35 @@ bool MacDrive::write(ReleaseVariant *data)
}
args.append(m_bsdDevice);

mCritical() << "The command is" << m_child->program() << args;
m_child->setArguments(args);
mCritical() << "The command is" << m_process->program() << args;
m_process->setArguments(args);

m_child->start();
m_process->start();

return true;
}

void MacDrive::cancel()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove MacDrive::cancel() completely. If we don't implement it ourself the base DriveManager::cancel() will be called.

{
Drive::cancel();
if (m_child) {
m_child->kill();
m_child->deleteLater();
m_child = nullptr;
}
// Reset the unique_ptr, which automatically invokes DriveOperationDeleter
m_process.reset();
}

void MacDrive::restore()
{
mCritical() << "starting to restore";
if (m_child)
m_child->deleteLater();

m_child = new QProcess(this);
// Create new process using unique_ptr
m_process.reset(new QProcess(this));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::make_unique


m_restoreStatus = RESTORING;
emit restoreStatusChanged();

if (QFile::exists(qApp->applicationDirPath() + "/../../../../helper/mac/helper.app/Contents/MacOS/helper")) {
m_child->setProgram(QString("%1/../../../../helper/mac/helper.app/Contents/MacOS/helper").arg(qApp->applicationDirPath()));
m_process->setProgram(QString("%1/../../../../helper/mac/helper.app/Contents/MacOS/helper").arg(qApp->applicationDirPath()));
} else if (QFile::exists(qApp->applicationDirPath() + "/helper")) {
m_child->setProgram(QString("%1/helper").arg(qApp->applicationDirPath()));
m_process->setProgram(QString("%1/helper").arg(qApp->applicationDirPath()));
} else {
m_restoreStatus = RESTORE_ERROR;
return;
Expand All @@ -155,16 +148,16 @@ void MacDrive::restore()
QStringList args;
args.append("restore");
args.append(m_bsdDevice);
m_child->setArguments(args);
m_process->setArguments(args);

m_child->setProcessChannelMode(QProcess::ForwardedChannels);
m_process->setProcessChannelMode(QProcess::ForwardedChannels);

connect(m_child, static_cast<void (QProcess::*)(int, QProcess::ExitStatus)>(&QProcess::finished), this, &MacDrive::onRestoreFinished);
connect(qApp, &QCoreApplication::aboutToQuit, m_child, &QProcess::terminate);
connect(m_process.get(), static_cast<void (QProcess::*)(int, QProcess::ExitStatus)>(&QProcess::finished), this, &MacDrive::onRestoreFinished);
connect(qApp, &QCoreApplication::aboutToQuit, m_process.get(), &QProcess::terminate);

mCritical() << "The command is" << m_child->program() << args;
mCritical() << "The command is" << m_process->program() << args;

m_child->start();
m_process->start();
}

void MacDrive::onFinished(int exitCode, QProcess::ExitStatus exitStatus)
Expand All @@ -173,11 +166,11 @@ void MacDrive::onFinished(int exitCode, QProcess::ExitStatus exitStatus)

setDelayedWrite(false);

if (!m_child)
if (!m_process)
return;

if (exitCode != 0) {
QString output = m_child->readAllStandardError();
QString output = m_process->readAllStandardError();
QRegularExpression re("^.+:.+: ");
QStringList lines = output.split('\n');
if (lines.length() > 0) {
Expand All @@ -194,25 +187,24 @@ void MacDrive::onFinished(int exitCode, QProcess::ExitStatus exitStatus)

void MacDrive::onRestoreFinished(int exitCode, QProcess::ExitStatus exitStatus)
{
if (!m_child)
if (!m_process)
return;

mCritical() << "Process finished" << exitCode << exitStatus;
mCritical() << m_child->readAllStandardError();
mCritical() << m_process->readAllStandardError();

if (exitCode == 0)
m_restoreStatus = RESTORED;
else
m_restoreStatus = RESTORE_ERROR;
emit restoreStatusChanged();

m_child->deleteLater();
m_child = nullptr;
// Process cleanup handled automatically by unique_ptr deleter
}

void MacDrive::onReadyRead()
{
if (!m_child)
if (!m_process)
return;

if (m_image->status() == ReleaseVariant::WRITING) {
Expand All @@ -223,9 +215,9 @@ void MacDrive::onReadyRead()
if (m_image->status() != ReleaseVariant::WRITE_VERIFYING && m_image->status() != ReleaseVariant::WRITING)
m_image->setStatus(ReleaseVariant::WRITING);

while (m_child->bytesAvailable() > 0) {
while (m_process->bytesAvailable() > 0) {
bool ok;
int64_t bytes = m_child->readLine().trimmed().toULongLong(&ok);
int64_t bytes = m_process->readLine().trimmed().toULongLong(&ok);
if (ok)
m_progress->setValue(bytes);
}
Expand Down
1 change: 0 additions & 1 deletion src/app/macdrivemanager.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ private slots:

private:
QString m_bsdDevice;
QProcess *m_child{nullptr};
};

#endif // MACDRIVEMANAGER_H
Loading