Differential D10410

Move task of cleaning up socket file to FdReceiver
ClosedPublic
Actions

Authored by chinmoyr on Feb 9 2018, 5:26 PM.

Details

Reviewers
ossi
dfaure
Group Reviewers
Frameworks
Commits
R241:3e7c84e877e5: Move task of cleaning up socket file to FdReceiver
Summary

The class SocketAddress is meant only for creating a proper socket address structure. It should not be handling
cleanup of the socket file. FdReceiver is better suited for this job.

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
chinmoyr created this revision.Feb 9 2018, 5:26 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 9 2018, 5:26 PM
chinmoyr requested review of this revision.Feb 9 2018, 5:26 PM
dfaure accepted this revision.Mar 4 2018, 10:15 AM
This revision is now accepted and ready to land.Mar 4 2018, 10:15 AM
ossi added inline comments.Mar 4 2018, 11:31 AM
src/ioslaves/file/fdreceiver.cpp
58

any particular reason not to use QFile::remove() here as well?

chinmoyr added inline comments.Mar 4 2018, 11:33 AM
src/ioslaves/file/fdreceiver.cpp
58

I didn't want to include QFile. That's the only reason.

ossi added inline comments.Mar 4 2018, 11:34 AM
src/ioslaves/file/fdreceiver.cpp
58

uhm, you're still using it in this very line, just differently ;)

chinmoyr added inline comments.Mar 4 2018, 11:42 AM
src/ioslaves/file/fdreceiver.cpp
58

*head-desk*
I think I have to get my eyes checked. I will change it right away.

ossi requested changes to this revision.May 6 2018, 9:16 AM

still needs update

This revision now requires changes to proceed.May 6 2018, 9:16 AM
chinmoyr updated this revision to Diff 33761.May 7 2018, 3:28 PM

Used QString::toLocal8Bit() instead of QFile::encodeName(). QFile wasn't supposed to be included in this commit.

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptMay 22 2018, 4:56 PM
dfaure accepted this revision.May 25 2018, 11:08 PM
ossi requested changes to this revision.May 27 2018, 9:59 AM

did you make sure that this is the only place where SocketAddress is used?

src/ioslaves/file/fdreceiver.cpp
42

do the first unlink right before here, so it's equivalent with the old code, just better structured.

58

that's a good addition, but it isn't logically part of this patch, because it adds a new feature (cleanup at exit) instead of only refactoring.

src/ioslaves/file/file_unix.cpp
87 ↗(On Diff #33761)

that's the wrong place, imo. leave it FdReceiver, so it's more local.

This revision now requires changes to proceed.May 27 2018, 9:59 AM
chinmoyr added a comment.May 27 2018, 10:40 AM

SocketAddress is used only in FdSender and FdRecevier.

src/ioslaves/file/fdreceiver.cpp
58

So shall I commit this change separately right after pushing this patch? Or after all the related patches are pushed?

chinmoyr updated this revision to Diff 34959.May 27 2018, 10:41 AM
chinmoyr marked 2 inline comments as done.
chinmoyr added a dependency: D9966: [KIO] Fix issues with sharing of file descriptor.

Do the first unlink in FdReceiver constructor

chinmoyr updated this revision to Diff 34960.May 27 2018, 10:44 AM

corrected the unlink position

ossi accepted this revision.May 27 2018, 11:00 AM

note that the commit message needs a minor adjustment now.

src/ioslaves/file/fdreceiver.cpp
58

the order really doesn't matter here.

This revision is now accepted and ready to land.May 27 2018, 11:00 AM
chinmoyr edited the summary of this revision. (Show Details)Jun 1 2018, 11:31 AM
chinmoyr retitled this revision from Move the task of cleaning up of socket file to file ioslave and FdReceiver to Move task of cleaning up socket file to FdReceiver.Jun 1 2018, 11:33 AM
Closed by commit R241:3e7c84e877e5: Move task of cleaning up socket file to FdReceiver (authored by chinmoyr). · Explain WhyJun 1 2018, 11:35 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 35321

View Options

src/ioslaves/file/fdreceiver.h

Loading...
View Options

src/ioslaves/file/fdreceiver.cpp

Loading...
View Options

src/ioslaves/file/sharefd_p.h

Loading...