From 543e2d34fcf2721b9bd7a767828217a24f4a89c7 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Mon, 2 Sep 2019 12:02:43 +0200 Subject: [PATCH 1/4] [libcalamares] [locale] Remove unused includes --- src/libcalamares/geoip/GeoIPXML.cpp | 1 - src/modules/locale/LocaleViewStep.cpp | 2 -- 2 files changed, 3 deletions(-) diff --git a/src/libcalamares/geoip/GeoIPXML.cpp b/src/libcalamares/geoip/GeoIPXML.cpp index d84eb3b21..256e774cf 100644 --- a/src/libcalamares/geoip/GeoIPXML.cpp +++ b/src/libcalamares/geoip/GeoIPXML.cpp @@ -20,7 +20,6 @@ #include "utils/Logger.h" -#include #include namespace CalamaresUtils diff --git a/src/modules/locale/LocaleViewStep.cpp b/src/modules/locale/LocaleViewStep.cpp index ee5c7699b..9dbcb5993 100644 --- a/src/modules/locale/LocaleViewStep.cpp +++ b/src/modules/locale/LocaleViewStep.cpp @@ -35,8 +35,6 @@ #include #include -#include -#include #include From f8356a6dcc7fa935af8e5cc681b68ab75a22d88d Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Mon, 2 Sep 2019 12:21:33 +0200 Subject: [PATCH 2/4] [libcalamares] Add an async get method - Mostly a "cheap" wrapper for a half-dozen boilerplate lines of Qt NAM code. --- src/libcalamares/network/Manager.cpp | 41 ++++++++++++++++++++++++++++ src/libcalamares/network/Manager.h | 9 ++++++ 2 files changed, 50 insertions(+) diff --git a/src/libcalamares/network/Manager.cpp b/src/libcalamares/network/Manager.cpp index 6c2d6caab..0cf5be20f 100644 --- a/src/libcalamares/network/Manager.cpp +++ b/src/libcalamares/network/Manager.cpp @@ -99,6 +99,40 @@ Manager::setCheckHasInternetUrl( const QUrl& url ) d->m_hasInternetUrl = url; } +/** @brief Does a request asynchronously, returns the (pending) reply + * + * The extra options for the request are taken from @p options, + * including the timeout setting. A timeout will cause the reply + * to abort. + * + * On failure, returns nullptr (e.g. bad URL, timeout). + */ +static QNetworkReply* +asynchronousRun( const std::unique_ptr< QNetworkAccessManager >& nam, const QUrl& url, const RequestOptions& options ) +{ + QNetworkRequest request = QNetworkRequest( url ); + QNetworkReply* reply = nam->get( request ); + QTimer* timer = nullptr; + + // Bail out early if the request is bad + if ( reply->error() ) + { + reply->deleteLater(); + return nullptr; + } + + options.applyToRequest( &request ); + if ( options.hasTimeout() ) + { + timer = new QTimer( reply ); + timer->setSingleShot( true ); + QObject::connect( timer, &QTimer::timeout, reply, &QNetworkReply::abort ); + timer->start( options.timeout() ); + } + + return reply; +} + /** @brief Does a request synchronously, returns the request itself * * The extra options for the request are taken from @p options, @@ -173,5 +207,12 @@ Manager::synchronousGet( const QUrl& url, const RequestOptions& options ) return reply.first ? reply.second->readAll() : QByteArray(); } +QNetworkReply* +Manager::asynchronouseGet( const QUrl& url, const CalamaresUtils::Network::RequestOptions& options ) +{ + return asynchronousRun( d->m_nam, url, options ); +} + + } // namespace Network } // namespace CalamaresUtils diff --git a/src/libcalamares/network/Manager.h b/src/libcalamares/network/Manager.h index 041314f13..62788f504 100644 --- a/src/libcalamares/network/Manager.h +++ b/src/libcalamares/network/Manager.h @@ -28,6 +28,7 @@ #include #include +class QNetworkReply; class QNetworkRequest; namespace CalamaresUtils @@ -139,6 +140,14 @@ public: */ bool hasInternet(); + /** @brief Do a network request asynchronously. + * + * Returns a pointer to the reply-from-the-request. + * This may be a nullptr if an error occurs immediately. + * The caller is responsible for cleaning up the reply (eventually). + */ + QNetworkReply* asynchronouseGet( const QUrl& url, const RequestOptions& options = RequestOptions() ); + private: struct Private; std::unique_ptr< Private > d; From badbdf59ee357483ef04eb338796c1f1df4a1f70 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Mon, 2 Sep 2019 13:43:10 +0200 Subject: [PATCH 3/4] [libcalamares] Build synchronous get w/ async-get --- src/libcalamares/network/Manager.cpp | 38 +++++++++++----------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/src/libcalamares/network/Manager.cpp b/src/libcalamares/network/Manager.cpp index 0cf5be20f..56e36cd55 100644 --- a/src/libcalamares/network/Manager.cpp +++ b/src/libcalamares/network/Manager.cpp @@ -103,7 +103,7 @@ Manager::setCheckHasInternetUrl( const QUrl& url ) * * The extra options for the request are taken from @p options, * including the timeout setting. A timeout will cause the reply - * to abort. + * to abort. The reply is **not** scheduled for deletion. * * On failure, returns nullptr (e.g. bad URL, timeout). */ @@ -144,36 +144,28 @@ asynchronousRun( const std::unique_ptr< QNetworkAccessManager >& nam, const QUrl static QPair< RequestStatus, QNetworkReply* > synchronousRun( const std::unique_ptr< QNetworkAccessManager >& nam, const QUrl& url, const RequestOptions& options ) { - QNetworkRequest request = QNetworkRequest( url ); - QNetworkReply* reply = nam->get( request ); - QEventLoop loop; - QTimer timer; - - // Bail out early if the request is bad - if ( reply->error() ) - { - reply->deleteLater(); - return qMakePair( RequestStatus( RequestStatus::Failed ), nullptr ); - } - - options.applyToRequest( &request ); - if ( options.hasTimeout() ) + auto* reply = asynchronousRun( nam, url, options ); + if ( !reply ) { - timer.setSingleShot( true ); - QObject::connect( &timer, &QTimer::timeout, &loop, &QEventLoop::quit ); - timer.start( options.timeout() ); + return qMakePair( RequestStatus( RequestStatus::Failed ), nullptr ); } + QEventLoop loop; QObject::connect( reply, &QNetworkReply::finished, &loop, &QEventLoop::quit ); loop.exec(); - if ( options.hasTimeout() && !timer.isActive() ) + reply->deleteLater(); + if ( reply->isRunning() ) { - reply->deleteLater(); return qMakePair( RequestStatus( RequestStatus::Timeout ), nullptr ); } - - reply->deleteLater(); - return qMakePair( RequestStatus( RequestStatus::Ok ), reply ); + else if ( reply->error() != QNetworkReply::NoError ) + { + return qMakePair( RequestStatus( RequestStatus::Timeout ), nullptr ); + } + else + { + return qMakePair( RequestStatus( RequestStatus::Ok ), reply ); + } } RequestStatus From 9850e4b35b0a7d0749e0803781f94f803de37294 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Mon, 2 Sep 2019 14:10:36 +0200 Subject: [PATCH 4/4] [netinstall] Use the network-manager - use asynchronousGet and keep the reply - drop unused includes - apply current coding style --- src/modules/netinstall/NetInstallPage.cpp | 91 ++++++++++++++--------- src/modules/netinstall/NetInstallPage.h | 19 ++--- 2 files changed, 62 insertions(+), 48 deletions(-) diff --git a/src/modules/netinstall/NetInstallPage.cpp b/src/modules/netinstall/NetInstallPage.cpp index a4f52d5b7..c3754e1c2 100644 --- a/src/modules/netinstall/NetInstallPage.cpp +++ b/src/modules/netinstall/NetInstallPage.cpp @@ -22,26 +22,24 @@ #include "NetInstallPage.h" #include "PackageModel.h" - #include "ui_page_netinst.h" + #include "JobQueue.h" +#include "network/Manager.h" #include "utils/Logger.h" #include "utils/Retranslator.h" #include "utils/Yaml.h" -#include -#include -#include - #include +#include using CalamaresUtils::yamlToVariant; NetInstallPage::NetInstallPage( QWidget* parent ) : QWidget( parent ) , ui( new Ui::Page_NetInst ) - , m_networkManager( this ) + , m_reply( nullptr ) , m_groups( nullptr ) { ui->setupUi( this ); @@ -55,14 +53,14 @@ NetInstallPage::readGroups( const QByteArray& yamlData ) YAML::Node groups = YAML::Load( yamlData.constData() ); if ( !groups.IsSequence() ) + { cWarning() << "netinstall groups data does not form a sequence."; + } Q_ASSERT( groups.IsSequence() ); m_groups = new PackageModel( groups ); - CALAMARES_RETRANSLATE( - m_groups->setHeaderData( 0, Qt::Horizontal, tr( "Name" ) ); - m_groups->setHeaderData( 1, Qt::Horizontal, tr( "Description" ) ); ) + CALAMARES_RETRANSLATE( m_groups->setHeaderData( 0, Qt::Horizontal, tr( "Name" ) ); + m_groups->setHeaderData( 1, Qt::Horizontal, tr( "Description" ) ); ) return true; - } catch ( YAML::Exception& e ) { @@ -71,29 +69,53 @@ NetInstallPage::readGroups( const QByteArray& yamlData ) } } +/// @brief Convenience to zero out and deleteLater on the reply, used in dataIsHere +struct ReplyDeleter +{ + QNetworkReply*& p; + + ~ReplyDeleter() + { + if ( p ) + { + p->deleteLater(); + } + p = nullptr; + } +}; + void -NetInstallPage::dataIsHere( QNetworkReply* reply ) +NetInstallPage::dataIsHere() { - cDebug() << "NetInstall group data received" << reply->url(); - reply->deleteLater(); + if ( !m_reply || !m_reply->isFinished() ) + { + cWarning() << "NetInstall data called too early."; + return; + } + + cDebug() << "NetInstall group data received" << m_reply->url(); + + ReplyDeleter d { m_reply }; // If m_required is *false* then we still say we're ready // even if the reply is corrupt or missing. - if ( reply->error() != QNetworkReply::NoError ) + if ( m_reply->error() != QNetworkReply::NoError ) { cWarning() << "unable to fetch netinstall package lists."; - cDebug() << Logger::SubEntry << "Netinstall reply error: " << reply->error(); - cDebug() << Logger::SubEntry << "Request for url: " << reply->url().toString() << " failed with: " << reply->errorString(); - ui->netinst_status->setText( tr( "Network Installation. (Disabled: Unable to fetch package lists, check your network connection)" ) ); + cDebug() << Logger::SubEntry << "Netinstall reply error: " << m_reply->error(); + cDebug() << Logger::SubEntry << "Request for url: " << m_reply->url().toString() + << " failed with: " << m_reply->errorString(); + ui->netinst_status->setText( + tr( "Network Installation. (Disabled: Unable to fetch package lists, check your network connection)" ) ); emit checkReady( !m_required ); return; } - if ( !readGroups( reply->readAll() ) ) + if ( !readGroups( m_reply->readAll() ) ) { cWarning() << "netinstall groups data was received, but invalid."; - cDebug() << Logger::SubEntry << "Url: " << reply->url().toString(); - cDebug() << Logger::SubEntry << "Headers: " << reply->rawHeaderList(); + cDebug() << Logger::SubEntry << "Url: " << m_reply->url().toString(); + cDebug() << Logger::SubEntry << "Headers: " << m_reply->rawHeaderList(); ui->netinst_status->setText( tr( "Network Installation. (Disabled: Received invalid groups data)" ) ); emit checkReady( !m_required ); return; @@ -110,7 +132,9 @@ PackageModel::PackageItemDataList NetInstallPage::selectedPackages() const { if ( m_groups ) + { return m_groups->getPackages(); + } else { cWarning() << "no netinstall groups are available."; @@ -121,24 +145,23 @@ NetInstallPage::selectedPackages() const void NetInstallPage::loadGroupList( const QString& confUrl ) { + using namespace CalamaresUtils::Network; + cDebug() << "NetInstall loading groups from" << confUrl; - QNetworkRequest request; - request.setUrl( QUrl( confUrl ) ); - // Follows all redirects except unsafe ones (https to http). - request.setAttribute( QNetworkRequest::FollowRedirectsAttribute, true ); - // Not everybody likes the default User Agent used by this class (looking at you, - // sourceforge.net), so let's set a more descriptive one. - request.setRawHeader( "User-Agent", "Mozilla/5.0 (compatible; Calamares)" ); - - connect( &m_networkManager, &QNetworkAccessManager::finished, - this, &NetInstallPage::dataIsHere ); - auto* rq = m_networkManager.get( request ); - if ( rq->error() ) + QNetworkReply* reply = Manager::instance().asynchronouseGet( + QUrl( confUrl ), + RequestOptions( RequestOptions::FakeUserAgent | RequestOptions::FollowRedirect, std::chrono::seconds( 30 ) ) ); + + if ( !reply ) { - cDebug() << Logger::Continuation << "request failed immediately," << rq->errorString(); - rq->deleteLater(); + cDebug() << Logger::Continuation << "request failed immediately."; ui->netinst_status->setText( tr( "Network Installation. (Disabled: Incorrect configuration)" ) ); } + else + { + m_reply = reply; + connect( reply, &QNetworkReply::finished, this, &NetInstallPage::dataIsHere ); + } } void diff --git a/src/modules/netinstall/NetInstallPage.h b/src/modules/netinstall/NetInstallPage.h index b2887304b..fbd60a8fd 100644 --- a/src/modules/netinstall/NetInstallPage.h +++ b/src/modules/netinstall/NetInstallPage.h @@ -24,14 +24,10 @@ #include "PackageModel.h" #include "PackageTreeItem.h" -#include -#include +#include #include -// required forward declarations -class QByteArray; class QNetworkReply; -class QString; namespace Ui { @@ -57,10 +53,7 @@ public: // corrupt or unavailable data causes checkReady() to be emitted // true (not-required) or false. void setRequired( bool ); - bool getRequired() const - { - return m_required; - } + bool getRequired() const { return m_required; } // Returns the list of packages belonging to groups that are // selected in the view in this given moment. No data is cached here, so @@ -68,7 +61,7 @@ public: PackageModel::PackageItemDataList selectedPackages() const; public slots: - void dataIsHere( QNetworkReply* ); + void dataIsHere(); signals: void checkReady( bool ); @@ -81,11 +74,9 @@ private: Ui::Page_NetInst* ui; - // Handles connection with the remote URL storing the configuration. - QNetworkAccessManager m_networkManager; - + QNetworkReply* m_reply; PackageModel* m_groups; bool m_required; }; -#endif // NETINSTALLPAGE_H +#endif // NETINSTALLPAGE_H