Commit 0da6ac9d authored by Harald Sitter's avatar Harald Sitter 🏳️‍🌈
Browse files

equip SMARTData with a validity check

this fixes vbox drives getting reported as possibly breaking when indeed
they simply lack any amount of smart data. currently I have no evidence
that this is a more wide spread issue beyond vbox so the validity
condition is fairly constrained.

this also sees a slight refactor of smartctl which previously had error
some weak failure handling, by moving that outside the kauth-dependent
code we can actually test this though and smartmonitor is easily retro
fitted for validity handling anyway
parent f83ec86a
{
"json_format_version": [
1,
0
],
"smartctl": {
"version": [
7,
1
],
"svn_revision": "5022",
"platform_info": "x86_64-linux-5.4.0-71-generic",
"build_info": "(local build)",
"argv": [
"smartctl",
"--all",
"--json",
"/dev/sda"
],
"exit_status": 4
},
"device": {
"name": "/dev/sda",
"info_name": "/dev/sda [SAT]",
"type": "sat",
"protocol": "ATA"
},
"model_name": "VBOX HARDDISK",
"serial_number": "VB1115a2a3-89df5dfe",
"firmware_version": "1.0",
"user_capacity": {
"blocks": 33554432,
"bytes": 17179869184
},
"logical_block_size": 512,
"physical_block_size": 512,
"in_smartctl_database": false,
"ata_version": {
"string": "ATA/ATAPI-6 published, ANSI INCITS 361-2002",
"major_value": 126,
"minor_value": 34
},
"local_time": {
"time_t": 1618305729,
"asctime": "Tue Apr 13 05:22:09 2021 EDT"
}
}
SPDX-License-Identifier: CC0-1.0
SPDX-FileCopyrightText: none
......@@ -12,5 +12,6 @@
<file alias='fail.json'>./fail.json</file>
<file alias='missing-status.json'>./missing-status.json</file>
<file alias='pass.json'>./pass.json</file>
<file alias='invalid-vbox.json'>./invalid-vbox.json</file>
</qresource>
</RCC>
......@@ -25,6 +25,7 @@ private Q_SLOTS:
QCOMPARE(data.m_device, "/dev/testfoobarpass");
QCOMPARE(data.m_status.m_passed, true);
QVERIFY(!data.m_smartctl.failure());
QVERIFY(data.m_valid);
}
void testFail()
......@@ -37,6 +38,7 @@ private Q_SLOTS:
QCOMPARE(data.m_device, "/dev/testfoobarfail");
QCOMPARE(data.m_status.m_passed, false);
QVERIFY(!data.m_smartctl.failure());
QVERIFY(data.m_valid);
}
void testBroken()
......@@ -52,6 +54,7 @@ private Q_SLOTS:
SMART::Failures({SMART::Failure::Disk, SMART::Failure::Prefail, SMART::Failure::ErrorsRecorded, SMART::Failure::SelfTestErrors}));
QVERIFY(data.m_smartctl.failure());
QVERIFY(!!data.m_smartctl.failure());
QVERIFY(data.m_valid);
}
void testTimeout()
......@@ -67,6 +70,7 @@ private Q_SLOTS:
QCOMPARE(data.m_smartctl.failure(), SMART::Failures({SMART::Failure::InternalCommand}));
QVERIFY(data.m_smartctl.failure());
QVERIFY(!!data.m_smartctl.failure());
QVERIFY(data.m_valid);
}
void testFailingSectorsButPassingStatus()
......@@ -79,6 +83,20 @@ private Q_SLOTS:
QCOMPARE(data.m_device, "/dev/sdb");
QCOMPARE(data.m_status.m_passed, true);
QCOMPARE(data.m_smartctl.failure(), SMART::Failures({SMART::Failure::ErrorsRecorded | SMART::Failure::SelfTestErrors}));
QVERIFY(data.m_valid);
}
void testVBoxDrive()
{
// VBox virtual drives fail with bit2 and have no status
QFile file(QFINDTESTDATA("fixtures/invalid-vbox.json"));
QVERIFY(file.open(QFile::ReadOnly));
auto doc = QJsonDocument::fromJson(file.readAll());
SMARTData data(doc);
QCOMPARE(data.m_device, "/dev/sda");
QCOMPARE(data.m_status.m_passed, false);
QCOMPARE(data.m_smartctl.failure(), SMART::Failures({SMART::Failure::InternalCommand}));
QVERIFY(!data.m_valid);
}
};
......
......@@ -27,7 +27,9 @@ private Q_SLOTS:
struct Ctl : public AbstractSMARTCtl {
void run(const QString &devicePath) override
{
static QMap<QString, QString> data{{"/dev/testfoobarpass", "fixtures/pass.json"}, {"/dev/testfoobarfail", "fixtures/fail.json"}};
static QMap<QString, QString> data{{"/dev/testfoobarpass", "fixtures/pass.json"},
{"/dev/invalid-vbox.json", "fixtures/invalid-vbox.json"},
{"/dev/testfoobarfail", "fixtures/fail.json"}};
const QString fixture = data.value(devicePath);
Q_ASSERT(!fixture.isEmpty());
......@@ -51,6 +53,7 @@ private Q_SLOTS:
void loadData() override
{
Q_EMIT addDevice(new Device{"udi-pass", "product", "/dev/testfoobarpass"});
Q_EMIT addDevice(new Device{"udi-invalid", "product", "/dev/invalid-vbox.json"});
// discover this twice to ensure notifications aren't duplicated!
Q_EMIT addDevice(new Device{"udi-fail", "product", "/dev/testfoobarfail"});
Q_EMIT addDevice(new Device{"udi-fail", "product", "/dev/testfoobarfail"});
......@@ -68,18 +71,23 @@ private Q_SLOTS:
QCOMPARE(monitor.devices().count(), 2); // There are 3 devices but one is a dupe.
bool sawPass = false;
bool sawInvalid = false;
bool sawFail = false;
for (const auto *device : monitor.devices()) {
if (device->path() == "/dev/testfoobarpass") {
QVERIFY(!device->failed());
sawPass = true;
}
if (device->path() == "/dev/invalid") {
sawInvalid = true;
}
if (device->path() == "/dev/testfoobarfail") {
QVERIFY(device->failed());
sawFail = true;
}
}
QVERIFY(sawPass);
QVERIFY(!sawInvalid); // mustn't be seen, it's an invalid device ;)
QVERIFY(sawFail);
// Ensure removing works as well.
......
......@@ -49,7 +49,7 @@ void SMARTCtl::run(const QString &devicePath)
const auto json = data.value(QStringLiteral("data"), QByteArray()).toByteArray();
QJsonDocument document;
if (json.isEmpty() || code & SMART::Failure::CmdLineParse || code & SMART::Failure::DeviceOpen) {
if (json.isEmpty()) {
qCDebug(KDED) << "looks like we got no data back for" << devicePath << code << json.isEmpty();
} else {
document = QJsonDocument::fromJson(json);
......
......@@ -7,6 +7,8 @@
#include <QJsonObject>
#include <QJsonValue>
#include "kded_debug.h"
SMARTStatus::SMARTStatus(const QJsonObject &object)
: m_passed(object[QStringLiteral("passed")].toBool())
{
......@@ -27,5 +29,28 @@ SMARTData::SMARTData(const QJsonDocument &document)
: m_smartctl(SMARTCtlData(document.object()[QStringLiteral("smartctl")].toObject()))
, m_status(SMARTStatus(document.object()[QStringLiteral("smart_status")].toObject()))
, m_device(document.object()[QStringLiteral("device")].toObject()[QStringLiteral("name")].toString())
, m_valid(checkValid(document))
{
}
bool SMARTData::checkValid(const QJsonDocument &document) const
{
if (m_smartctl.failure() & SMART::Failure::CmdLineParse) {
qCDebug(KDED) << "Command line error" << m_device << document.toJson();
return false;
}
if (m_smartctl.failure() & SMART::Failure::DeviceOpen) {
qCDebug(KDED) << "Failed to open device" << m_device << document.toJson();
return false;
}
const bool hasSMARTStatus = document.object().contains(QStringLiteral("smart_status"));
const bool internalCommandFailure = (m_smartctl.failure() & SMART::Failure::InternalCommand);
if (!hasSMARTStatus && internalCommandFailure) {
// VirtualBox drives return with InternalCommand problems and no SMART data. Consider the data invalid.
// If we also have other failure codes we'll still want to consider the data valid as it might indicate
// problems.
qCDebug(KDED) << "Internal command problems resulted in no smart_status data" << m_device << document.toJson();
return false;
}
return true;
}
......@@ -9,8 +9,8 @@
#include "smartfailure.h"
class QJsonObject;
class QJsonDocument;
class QJsonObject;
/** Models "smart_status" blobs */
class SMARTStatus
......@@ -42,6 +42,14 @@ public:
SMARTCtlData m_smartctl;
SMARTStatus m_status;
QString m_device;
// Keep at end so it's initialized last. m_valid is initialized
// from checkValid() and that requires other members to have been
// initialized already.
const bool m_valid = false;
private:
bool checkValid(const QJsonDocument &document) const;
};
#endif // SMARTDATA_H
......@@ -66,6 +66,7 @@ void SMARTMonitor::onSMARTCtlFinished(const QString &devicePath, const QJsonDocu
m_pendingDevices.erase(pendingIt);
if (document.isEmpty()) { // failed to get data, ignore the device
qCDebug(KDED) << "Received no data for" << devicePath;
device->deleteLater();
return;
}
......@@ -75,6 +76,12 @@ void SMARTMonitor::onSMARTCtlFinished(const QString &devicePath, const QJsonDocu
Q_ASSERT(devicePath == data.m_device);
}
if (!data.m_valid) {
qCDebug(KDED) << "Invalid SMART data; skipping" << devicePath;
device->deleteLater();
return;
}
auto existingIt = std::find_if(m_devices.begin(), m_devices.end(), [&device](Device *existing) {
return *existing == *device;
});
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment