From fd183cb40b2a5d1e1bb0175c68dc5b88a7d3ffc3 Mon Sep 17 00:00:00 2001 From: John Thacker Date: Sat, 7 Jan 2023 09:04:41 -0500 Subject: [PATCH] Qt: Add ability to cancel sorting Add the ability to cancel sorting. Since we now parse user inputs during the sort, test and set the capture file read lock. Try to sort in PacketList::captureFileReadFinished, since now sorting during thawing won't happen if it's in the middle of a rescan. Fix #17640 --- file.c | 17 ++--- ui/qt/models/packet_list_model.cpp | 110 +++++++++++++++++++++++------ ui/qt/models/packet_list_model.h | 8 +++ ui/qt/packet_list.cpp | 7 ++ 4 files changed, 114 insertions(+), 28 deletions(-) diff --git a/file.c b/file.c index 24bb3ba71b..b1cd7dd26b 100644 --- a/file.c +++ b/file.c @@ -689,6 +689,11 @@ cf_read(capture_file *cf, gboolean reloading) cf->current_frame = frame_data_sequence_find(cf->provider.frames, cf->first_displayed); packet_list_thaw(); + + /* It is safe again to execute redissections or sort. */ + ws_assert(cf->read_lock); + cf->read_lock = FALSE; + if (reloading) cf_callback_invoke(cf_cb_file_reload_finished, cf); else @@ -700,10 +705,6 @@ cf_read(capture_file *cf, gboolean reloading) packet_list_select_row_from_data(NULL); } - /* It is safe again to execute redissections. */ - ws_assert(cf->read_lock); - cf->read_lock = FALSE; - if (is_read_aborted) { /* * Well, the user decided to exit Wireshark while reading this *offline* @@ -1934,6 +1935,10 @@ rescan_packets(capture_file *cf, const char *action, const char *action_item, gb packet_list_thaw(); + /* It is safe again to execute redissections or sort. */ + ws_assert(cf->read_lock); + cf->read_lock = FALSE; + cf_callback_invoke(cf_cb_file_rescan_finished, cf); if (selected_frame_num == -1) { @@ -1997,10 +2002,6 @@ rescan_packets(capture_file *cf, const char *action, const char *action_item, gb /* Cleanup and release all dfilter resources */ dfilter_free(dfcode); - /* It is safe again to execute redissections. */ - ws_assert(cf->read_lock); - cf->read_lock = FALSE; - /* If another rescan (due to dfilter change) or redissection (due to profile * change) was requested, the rescan above is aborted and restarted here. */ if (queued_rescan_type != RESCAN_NONE) { diff --git a/ui/qt/models/packet_list_model.cpp b/ui/qt/models/packet_list_model.cpp index 7c71e824c6..aa865ee24c 100644 --- a/ui/qt/models/packet_list_model.cpp +++ b/ui/qt/models/packet_list_model.cpp @@ -9,6 +9,8 @@ #include #include +#include +#include #include "packet_list_model.h" @@ -44,6 +46,11 @@ #include #endif +class SortAbort : public std::runtime_error +{ + using std::runtime_error::runtime_error; +}; + static PacketListModel * glbl_plist_model = Q_NULLPTR; static const int reserved_packets_ = 100000; @@ -340,6 +347,10 @@ int PacketListModel::sort_column_is_numeric_; int PacketListModel::text_sort_column_; Qt::SortOrder PacketListModel::sort_order_; capture_file *PacketListModel::sort_cap_file_; +gboolean PacketListModel::stop_flag_; +ProgressFrame *PacketListModel::progress_frame_; +double PacketListModel::comps_; +double PacketListModel::exp_comps_; QElapsedTimer busy_timer_; const int busy_timeout_ = 65; // ms, approximately 15 fps @@ -379,43 +390,90 @@ void PacketListModel::sort(int column, Qt::SortOrder order) return; } - // XXX Use updateProgress instead. We'd have to switch from std::sort to - // something we can interrupt. + /* If we are currently in the middle of reading the capture file, don't + * sort. PacketList::captureFileReadFinished invalidates all the cached + * column strings and then tries to sort again. + * Similarly, claim the read lock because we don't want the file to + * change out from under us while sorting, which can segfault. (Previously + * we ignored user input, but now in order to cancel sorting we don't.) + */ + if (sort_cap_file_->read_lock) { + ws_info("Refusing to sort because capture file is being read"); + /* We shouldn't have to tell the user because we're just deferring + * the sort until PacketList::captureFileReadFinished + */ + return; + } + sort_cap_file_->read_lock = TRUE; + + QString busy_msg; if (!col_title.isEmpty()) { - QString busy_msg = tr("Sorting \"%1\"…").arg(col_title); - mainApp->pushStatus(MainApplication::BusyStatus, busy_msg); + busy_msg = tr("Sorting \"%1\"…").arg(col_title); + } else { + busy_msg = tr("Sorting …"); + } + stop_flag_ = FALSE; + comps_ = 0; + /* XXX: The expected number of comparisons is O(N log N), but this could + * be a pretty significant overestimate of the amount of time it takes, + * if there are lots of identical entries. (Especially with string + * comparisons, some comparisons are faster than others.) Better to + * overestimate? + */ + exp_comps_ = log2(visible_rows_.count()) * visible_rows_.count(); + progress_frame_ = nullptr; + if (qobject_cast(mainApp->mainWindow())) { + MainWindow *mw = qobject_cast(mainApp->mainWindow()); + progress_frame_ = mw->findChild(); + if (progress_frame_) { + progress_frame_->showProgress(busy_msg, true, false, &stop_flag_, 0); + connect(progress_frame_, &ProgressFrame::stopLoading, + this, &PacketListModel::stopSorting); + } } busy_timer_.start(); sort_column_is_numeric_ = isNumericColumn(sort_column_); QVector sorted_visible_rows_ = visible_rows_; - std::sort(sorted_visible_rows_.begin(), sorted_visible_rows_.end(), recordLessThan); + try { + std::sort(sorted_visible_rows_.begin(), sorted_visible_rows_.end(), recordLessThan); - beginResetModel(); - visible_rows_.resize(0); - number_to_row_.fill(0); - foreach (PacketListRecord *record, sorted_visible_rows_) { - frame_data *fdata = record->frameData(); + beginResetModel(); + visible_rows_.resize(0); + number_to_row_.fill(0); + foreach (PacketListRecord *record, sorted_visible_rows_) { + frame_data *fdata = record->frameData(); - if (fdata->passed_dfilter || fdata->ref_time) { - visible_rows_ << record; - if (number_to_row_.size() <= (int)fdata->num) { - number_to_row_.resize(fdata->num + 10000); + if (fdata->passed_dfilter || fdata->ref_time) { + visible_rows_ << record; + if (number_to_row_.size() <= (int)fdata->num) { + number_to_row_.resize(fdata->num + 10000); + } + number_to_row_[fdata->num] = static_cast(visible_rows_.count()); } - number_to_row_[fdata->num] = static_cast(visible_rows_.count()); } + endResetModel(); + } catch (const SortAbort& e) { + mainApp->pushStatus(MainApplication::TemporaryStatus, e.what()); } - endResetModel(); - if (!col_title.isEmpty()) { - mainApp->popStatus(MainApplication::BusyStatus); + if (progress_frame_ != nullptr) { + progress_frame_->hide(); + disconnect(progress_frame_, &ProgressFrame::stopLoading, + this, &PacketListModel::stopSorting); } + sort_cap_file_->read_lock = FALSE; if (cap_file_->current_frame) { emit goToPacket(cap_file_->current_frame->num); } } +void PacketListModel::stopSorting() +{ + stop_flag_ = TRUE; +} + bool PacketListModel::isNumericColumn(int column) { if (column < 0) { @@ -486,15 +544,22 @@ bool PacketListModel::isNumericColumn(int column) bool PacketListModel::recordLessThan(PacketListRecord *r1, PacketListRecord *r2) { int cmp_val = 0; + comps_++; // Wherein we try to cram the logic of packet_list_compare_records, // _packet_list_compare_records, and packet_list_compare_custom from // gtk/packet_list_store.c into one function if (busy_timer_.elapsed() > busy_timeout_) { + if (progress_frame_) { + progress_frame_->setValue(static_cast(comps_/exp_comps_ * 100)); + } // What's the least amount of processing that we can do which will draw // the busy indicator? - mainApp->processEvents(QEventLoop::ExcludeUserInputEvents | QEventLoop::ExcludeSocketNotifiers, 1); + mainApp->processEvents(QEventLoop::ExcludeSocketNotifiers, 1); + if (stop_flag_) { + throw SortAbort("Sorting aborted"); + } busy_timer_.restart(); } if (sort_column_ < 0) { @@ -506,11 +571,16 @@ bool PacketListModel::recordLessThan(PacketListRecord *r1, PacketListRecord *r2) } else { QString r1String = r1->columnString(sort_cap_file_, sort_column_); QString r2String = r2->columnString(sort_cap_file_, sort_column_); + // XXX: The naive string comparison compares Unicode code points. + // Proper collation is more expensive cmp_val = r1String.compare(r2String); if (cmp_val != 0 && sort_column_is_numeric_) { // Custom column with numeric data (or something like a port number). // Attempt to convert to numbers. - // XXX This is slow. Can we avoid doing this? + // XXX This is slow. Can we avoid doing this? Perhaps the actual + // values used for sorting should be cached too as QVariant[List]. + // If so, we could consider using QCollatorSortKeys or similar + // for strings as well. bool ok_r1, ok_r2; double num_r1 = parseNumericColumn(r1String, &ok_r1); double num_r2 = parseNumericColumn(r2String, &ok_r2); diff --git a/ui/qt/models/packet_list_model.h b/ui/qt/models/packet_list_model.h index c0060e4129..6f44dd5752 100644 --- a/ui/qt/models/packet_list_model.h +++ b/ui/qt/models/packet_list_model.h @@ -22,6 +22,8 @@ #include #include +#include + #include "packet_list_record.h" #include "cfile.h" @@ -84,6 +86,7 @@ signals: public slots: void sort(int column, Qt::SortOrder order = Qt::AscendingOrder); + void stopSorting(); void flushVisibleRows(); void dissectIdle(bool reset = false); @@ -106,6 +109,11 @@ private: static bool recordLessThan(PacketListRecord *r1, PacketListRecord *r2); static double parseNumericColumn(const QString &val, bool *ok); + static gboolean stop_flag_; + static ProgressFrame *progress_frame_; + static double exp_comps_; + static double comps_; + QElapsedTimer *idle_dissection_timer_; int idle_dissection_row_; diff --git a/ui/qt/packet_list.cpp b/ui/qt/packet_list.cpp index 7e9bf47a49..361d180d07 100644 --- a/ui/qt/packet_list.cpp +++ b/ui/qt/packet_list.cpp @@ -1229,6 +1229,10 @@ void PacketList::captureFileReadFinished() // Invalidating the column strings picks up and request/response // tracking changes. We might just want to call it from flushVisibleRows. packet_list_model_->invalidateAllColumnStrings(); + // Sort *after* invalidating the column strings + if (isSortingEnabled()) { + sortByColumn(header()->sortIndicatorSection(), header()->sortIndicatorOrder()); + } } void PacketList::freeze() @@ -1249,6 +1253,9 @@ void PacketList::freeze() void PacketList::thaw(bool restore_selection) { setHeaderHidden(false); + // Note that if we have a current sort status set in the header, + // this will automatically try to sort the model (we don't want + // that to happen if we're in the middle of reading the file). setModel(packet_list_model_); // Resetting the model resets our column widths so we restore them here.