Differential D22889 Diff 63137 src/util/externalcommandhelper.cpp

Changeset View

Standalone View

View Options

src/util/externalcommandhelper.cpp

Show First 20 Lines • Show All 52 Lines • ▼ Show 20 Line(s)50/** Reads the given number of bytes from the sourceDevice into the given buffer.
53 @return zero on success, non-zero on failure 53​ @return zero on success, non-zero on failure
54*/ 54​*/
55int ExternalCommandHelper::helperMain(int argc, char **argv) 55​int ExternalCommandHelper::helperMain(int argc, char **argv)
56{ 56​{
57 QCoreApplication app(argc, argv); 57​ QCoreApplication app(argc, argv);
58 58​
59 if (!QDBusConnection::systemBus().isConnected() || 59​ if (!QDBusConnection::systemBus().isConnected() ||
60 !QDBusConnection::systemBus().registerService(QStringLiteral("org.kde.kpmcore.helperinterface")) || 60​ !QDBusConnection::systemBus().registerService(QStringLiteral("org.kde.kpmcore.helperinterface")) ||
61 !QDBusConnection::systemBus().registerObject(QStringLiteral("/Helper"), this, QDBusConnection::ExportAllSlots)) { 61​ !QDBusConnection::systemBus().registerService(QStringLiteral("org.kde.kpmcore.applicationinterface")) ||
62​ !QDBusConnection::systemBus().registerObject(QStringLiteral("/Helper"), this, QDBusConnection::ExportAllSlots) ||
broulikUnsubmitted
Done

One should register objects before registering the service as the service might become visible to the bus in an inconsistent state (i.e. show up before the objects are registered)

broulik: One should register objects before registering the service as the service might become visible…
63​ !QDBusConnection::systemBus().registerObject(QStringLiteral("/Application"), this, QDBusConnection::ExportAllSlots)) {
cjlcarvalhoUnsubmitted
Not Done

What happens when a service/object is registered and the other one is not?

cjlcarvalho: What happens when a service/object is registered and the other one is not?
64​
62 qDebug() << "Failed to initialize the Helper"; 65​ qDebug() << "Failed to initialize the Helper";
63 qWarning() << QDBusConnection::systemBus().lastError().message(); 66​ qWarning() << QDBusConnection::systemBus().lastError().message();
64 67​
65 ​
66 // We have no reason to live when Main GUI app has expired 68​ // We have no reason to live when Main GUI app has expired
67 ​
68 ​
69 qApp->quit(); 69​ qApp->quit();
70 70​
71 return app.exec(); 71​ return app.exec();
72 } 72​ }
73 73​
74 m_loop = std::make_unique<QEventLoop>(); 74​ m_loop = std::make_unique<QEventLoop>();
75 emit reportProgress(QVariantMap()); 75​
76​ // We send zero percent new data on initial start-up
77​ sendProgress(0);
76 78​
77 // QDBus Service watcher which keeps an eye on the client (Main GUI app) 79​ // QDBus Service watcher which keeps an eye on the client (Main GUI app)
78 // End the loop and return only once the client has unregistered over the QDBus. 80​ // End the loop and return only once the client has unregistered over the QDBus.
79 auto serviceWatcher = new QDBusServiceWatcher(QStringLiteral("org.kde.kpmcore.applicationinterface"), 81​ auto serviceWatcher = new QDBusServiceWatcher(QStringLiteral("org.kde.kpmcore.applicationinterface"),
80 QDBusConnection::systemBus(), 82​ QDBusConnection::systemBus(),
81 QDBusServiceWatcher::WatchForUnregistration, 83​ QDBusServiceWatcher::WatchForUnregistration,
82 this); 84​ this);
83 85​
84 connect(serviceWatcher, &QDBusServiceWatcher::serviceUnregistered, 86​ connect(serviceWatcher, &QDBusServiceWatcher::serviceUnregistered,
85 [this]() { 87​ [this]() {
86 m_loop->exit(); 88​ m_loop->exit();
87 }); 89​ });
88 90​
89 m_loop->exec(); 91​ m_loop->exec();
90 92​
91 qApp->quit(); 93​ qApp->quit();
92 94​
93 return app.exec(); 95​ return app.exec();
94} 96​}
95 97​
96 ​
97/** Reads the given number of bytes from the sourceDevice into the given buffer. 98​/** Reads the given number of bytes from the sourceDevice into the given buffer.
98 @param sourceDevice device or file to read from 99​ @param sourceDevice device or file to read from
99 @param buffer buffer to store the bytes read in 100​ @param buffer buffer to store the bytes read in
100 @param offset offset where to begin reading 101​ @param offset offset where to begin reading
101 @param size the number of bytes to read 102​ @param size the number of bytes to read
102 @return true on success 103​ @return true on success
103*/ 104​*/
104bool ExternalCommandHelper::readData(const QString& sourceDevice, QByteArray& buffer, const qint64 offset, const qint64 size) 105​bool ExternalCommandHelper::readData(const QString& sourceDevice, QByteArray& buffer, const qint64 offset, const qint64 size)
▲ Show 20 Lines • Show All 43 Lines • ▼ Show 20 Line(s)136{
148 if (device.write(buffer) != buffer.size()) { 149​ if (device.write(buffer) != buffer.size()) {
149 qCritical() << xi18n("Could not write to device <filename>%1</filename>.", targetDevice); 150​ qCritical() << xi18n("Could not write to device <filename>%1</filename>.", targetDevice);
150 return false; 151​ return false;
151 } 152​ }
152 153​
153 return true; 154​ return true;
154} 155​}
155 156​
157​/** Makes asynchronous call to the main application.
broulikUnsubmitted
Done

This is a lie.

broulik: This is a lie.
158​ @param percent Percent of job completed.
159​*/
160​void ExternalCommandHelper::sendProgress(int percent)
161​{
162​ QDBusInterface *iface = new QDBusInterface(QStringLiteral("org.kde.kpmcore.applicationinterface"), QStringLiteral("/Application"), QStringLiteral("org.kde.kpmcore.externalcommand"), QDBusConnection::systemBus());
broulikUnsubmitted
Done

QDBusInterface constructor blocks introspecting DBus. You probably want to use an interface generated from an XML file to avoid this.

broulik: `QDBusInterface` constructor blocks introspecting DBus. You probably want to use an interface…
163​
164​ if (!iface->isValid()) {
165​ return;
166​ }
167​
168​ iface->setTimeout(10 * 24 * 3600 * 1000);
169​
170​ QDBusPendingCall pcall = iface->asyncCall(QLatin1String("emitNewData"), percent);
171​
172​ QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(pcall, this);
173​ QEventLoop loop;
broulikUnsubmitted
Not Done

What's the point of the event loop?

broulik: What's the point of the event loop?
174​
175​ auto exitLoop = [&] (QDBusPendingCallWatcher *watcher) {
176​ loop.exit();
177​
178​ if (watcher->isError())
179​ qWarning() << watcher->error();
180​ };
181​
182​ connect(watcher, &QDBusPendingCallWatcher::finished, exitLoop);
183​ loop.exec();
184​}
185​
186​/** Makes asynchronous call to the main application.
187​ @param message Message to send to the main application.
188​*/
189​void ExternalCommandHelper::sendProgress(QString message)
190​{
191​ QDBusInterface *iface = new QDBusInterface(QStringLiteral("org.kde.kpmcore.applicationinterface"), QStringLiteral("/Application"), QStringLiteral("org.kde.kpmcore.externalcommand"), QDBusConnection::systemBus());
192​
193​ if (!iface->isValid()) {
194​ return;
195​ }
196​
197​ iface->setTimeout(10 * 24 * 3600 * 1000);
198​
199​ QDBusPendingCall pcall = iface->asyncCall(QLatin1String("emitNewData"), message);
200​
201​ QDBusPendingCallWatcher *watcher = new QDBusPendingCallWatcher(pcall, this);
broulikUnsubmitted
Done

You're leaking the watcher, call watcher->deleteLater() in the finished handler

broulik: You're leaking the watcher, call `watcher->deleteLater()` in the `finished` handler
202​ QEventLoop loop;
203​
204​ auto exitLoop = [&] (QDBusPendingCallWatcher *watcher) {
205​ loop.exit();
206​
207​ if (watcher->isError())
208​ qWarning() << watcher->error();
209​ };
210​
211​ connect(watcher, &QDBusPendingCallWatcher::finished, exitLoop);
212​ loop.exec();
213​}
214​
156// If targetDevice is empty then return QByteArray with data that was read from disk. 215​// If targetDevice is empty then return QByteArray with data that was read from disk.
157QVariantMap ExternalCommandHelper::copyblocks(const QString& sourceDevice, const qint64 sourceFirstByte, const qint64 sourceLength, const QString& targetDevice, const qint64 targetFirstByte, const qint64 blockSize) 216​QVariantMap ExternalCommandHelper::copyblocks(const QString& sourceDevice, const qint64 sourceFirstByte, const qint64 sourceLength, const QString& targetDevice, const qint64 targetFirstByte, const qint64 blockSize)
158{ 217​{
159 QVariantMap reply; 218​ QVariantMap reply;
160 reply[QStringLiteral("success")] = true; 219​ reply[QStringLiteral("success")] = true;
161 220​
162 const qint64 blocksToCopy = sourceLength / blockSize; 221​ const qint64 blocksToCopy = sourceLength / blockSize;
163 qint64 readOffset = sourceFirstByte; 222​ qint64 readOffset = sourceFirstByte;
164 qint64 writeOffset = targetFirstByte; 223​ qint64 writeOffset = targetFirstByte;
165 qint32 copyDirection = 1; 224​ qint32 copyDirection = 1;
166 225​
167 if (targetFirstByte > sourceFirstByte) { 226​ if (targetFirstByte > sourceFirstByte) {
168 readOffset = sourceFirstByte + sourceLength - blockSize; 227​ readOffset = sourceFirstByte + sourceLength - blockSize;
169 writeOffset = targetFirstByte + sourceLength - blockSize; 228​ writeOffset = targetFirstByte + sourceLength - blockSize;
170 copyDirection = -1; 229​ copyDirection = -1;
171 } 230​ }
172 231​
173 const qint64 lastBlock = sourceLength % blockSize; 232​ const qint64 lastBlock = sourceLength % blockSize;
174 233​
175 qint64 bytesWritten = 0; 234​ qint64 bytesWritten = 0;
176 qint64 blocksCopied = 0; 235​ qint64 blocksCopied = 0;
177 236​
178 QByteArray buffer; 237​ QByteArray buffer;
179 int percent = 0; 238​ int percent = 0;
180 QTime t; ​
181 239​
240​ QTime t;
broulikUnsubmitted
Done

This probably wants to be a QElapsedTimer

broulik: This probably wants to be a `QElapsedTimer`
182 t.start(); 241​ t.start();
183 242​
184 QVariantMap report; 243​ sendProgress(xi18nc("@info:progress", "Copying %1 blocks (%2 bytes) from %3 to %4, direction: %5.", blocksToCopy,
185 ​
186 report[QStringLiteral("report")] = xi18nc("@info:progress", "Copying %1 blocks (%2 bytes) from %3 to %4, direction: %5.", blocksToCopy, ​
187 sourceLength, readOffset, writeOffset, copyDirection == 1 ? i18nc("direction: left", "left") 244​ sourceLength, readOffset, writeOffset, copyDirection == 1 ? i18nc("direction: left", "left")
188 : i18nc("direction: right", "right")); 245​ : i18nc("direction: right", "right")));
189 ​
190 emit reportProgress(report); ​
191 246​
192 bool rval = true; 247​ bool rval = true;
193 248​
194 while (blocksCopied < blocksToCopy && !targetDevice.isEmpty()) { 249​ while (blocksCopied < blocksToCopy && !targetDevice.isEmpty()) {
195 if (!(rval = readData(sourceDevice, buffer, readOffset + blockSize * blocksCopied * copyDirection, blockSize))) 250​ if (!(rval = readData(sourceDevice, buffer, readOffset + blockSize * blocksCopied * copyDirection, blockSize)))
196 break; 251​ break;
197 252​
198 if (!(rval = writeData(targetDevice, buffer, writeOffset + blockSize * blocksCopied * copyDirection))) 253​ if (!(rval = writeData(targetDevice, buffer, writeOffset + blockSize * blocksCopied * copyDirection)))
199 break; 254​ break;
200 255​
201 bytesWritten += buffer.size(); 256​ bytesWritten += buffer.size();
202 257​
203 if (++blocksCopied * 100 / blocksToCopy != percent) { 258​ if (++blocksCopied * 100 / blocksToCopy != percent) {
204 percent = blocksCopied * 100 / blocksToCopy; 259​ percent = blocksCopied * 100 / blocksToCopy;
205 260​
206 if (percent % 5 == 0 && t.elapsed() > 1000) { 261​ if (percent % 5 == 0 && t.elapsed() > 1000) {
207 const qint64 mibsPerSec = (blocksCopied * blockSize / 1024 / 1024) / (t.elapsed() / 1000); 262​ const qint64 mibsPerSec = (blocksCopied * blockSize / 1024 / 1024) / (t.elapsed() / 1000);
208 const qint64 estSecsLeft = (100 - percent) * t.elapsed() / percent / 1000; 263​ const qint64 estSecsLeft = (100 - percent) * t.elapsed() / percent / 1000;
209 report[QStringLiteral("report")]= xi18nc("@info:progress", "Copying %1 MiB/second, estimated time left: %2", mibsPerSec, QTime(0, 0).addSecs(estSecsLeft).toString()); 264​
210 emit reportProgress(report); 265​ sendProgress(xi18nc("@info:progress", "Copying %1 MiB/second, estimated time left: %2", mibsPerSec, QTime(0, 0).addSecs(estSecsLeft).toString()));
266​
211 } 267​ }
212 emit progress(percent); 268​ sendProgress(percent);
213 } 269​ }
214 } 270​ }
215 271​
216 // copy the remainder 272​ // copy the remainder
217 if (rval && lastBlock > 0) { 273​ if (rval && lastBlock > 0) {
218 Q_ASSERT(lastBlock < blockSize); 274​ Q_ASSERT(lastBlock < blockSize);
219 275​
220 const qint64 lastBlockReadOffset = copyDirection > 0 ? readOffset + blockSize * blocksCopied : sourceFirstByte; 276​ const qint64 lastBlockReadOffset = copyDirection > 0 ? readOffset + blockSize * blocksCopied : sourceFirstByte;
221 const qint64 lastBlockWriteOffset = copyDirection > 0 ? writeOffset + blockSize * blocksCopied : targetFirstByte; 277​ const qint64 lastBlockWriteOffset = copyDirection > 0 ? writeOffset + blockSize * blocksCopied : targetFirstByte;
222 278​
223 report[QStringLiteral("report")]= xi18nc("@info:progress", "Copying remainder of block size %1 from %2 to %3.", lastBlock, lastBlockReadOffset, lastBlockWriteOffset); 279​ sendProgress(xi18nc("@info:progress", "Copying remainder of block size %1 from %2 to %3.", lastBlock, lastBlockReadOffset, lastBlockWriteOffset));
224 ​
225 emit reportProgress(report); ​
226 280​
227 rval = readData(sourceDevice, buffer, lastBlockReadOffset, lastBlock); 281​ rval = readData(sourceDevice, buffer, lastBlockReadOffset, lastBlock);
228 282​
229 if (rval) { 283​ if (rval) {
230 if (targetDevice.isEmpty()) 284​ if (targetDevice.isEmpty())
231 reply[QStringLiteral("targetByteArray")] = buffer; 285​ reply[QStringLiteral("targetByteArray")] = buffer;
232 else 286​ else
233 rval = writeData(targetDevice, buffer, lastBlockWriteOffset); 287​ rval = writeData(targetDevice, buffer, lastBlockWriteOffset);
234 } 288​ }
235 289​
stikonasUnsubmitted
Done

Most of this code (except for asyncCall is the same in all cases).

It would make sense to factor it out into a separate function which might e.g. take an argument that is passed to emitNewData.

If you do just this you might still need two function (one for int overload that passed progress percents, one for the other call with QStrings).

It might be possible to unify it more but at leat do that...

stikonas: Most of this code (except for asyncCall is the same in all cases). It would make sense to…
236 if (rval) { 290​ if (rval) {
237 emit progress(100); 291​ sendProgress(100);
238 bytesWritten += buffer.size(); 292​ bytesWritten += buffer.size();
239 } 293​ }
240 } 294​ }
241 295​
242 report[QStringLiteral("report")] = xi18ncp("@info:progress argument 2 is a string such as 7 bytes (localized accordingly)", "Copying 1 block (%2) finished.", "Copying %1 blocks (%2) finished.", blocksCopied, i18np("1 byte", "%1 bytes", bytesWritten)); 296​ sendProgress(xi18ncp("@info:progress argument 2 is a string such as 7 bytes (localized accordingly)", "Copying 1 block (%2) finished.", "Copying %1 blocks (%2) finished.", blocksCopied, i18np("1 byte", "%1 bytes", bytesWritten)));
243 ​
244 emit reportProgress(report); ​
245 297​
246 reply[QStringLiteral("success")] = rval; 298​ reply[QStringLiteral("success")] = rval;
247 return reply; 299​ return reply;
248} 300​}
249 301​
250bool ExternalCommandHelper::writeData(const QByteArray& buffer, const QString& targetDevice, const qint64 targetFirstByte) 302​bool ExternalCommandHelper::writeData(const QByteArray& buffer, const QString& targetDevice, const qint64 targetFirstByte)
251{ 303​{
252 // Do not allow using this helper for writing to arbitrary location 304​ // Do not allow using this helper for writing to arbitrary location
▲ Show 20 Lines • Show All 69 Lines • Show Last 20 Lines