[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: Calling finished() after error() in KIO slaves causes data
From: Vlad <vladc6 () yahoo ! com>
Date: 2008-03-18 22:06:42
Message-ID: 71237.20899.qm () web54410 ! mail ! yahoo ! com
[Download RAW message or body]
Hi,
--- David Faure <faure@kde.org> wrote:
> On Tuesday 18 March 2008, Vlad wrote:
> > Makes sense, but I think the documentation for SlaveBase::error()
> > should be stricter, such as:
> >
> > "This also finishes the job, so you must not call finished after
> > calling this."
> >
> > I'll update this message if that's OK. I'll also add a comment to
> > SlaveBase::finished().
>
> Yes please.
>
> It should be enough in SlaveBase to have a member var with three
> states:
> enum { Idle, InsideMethod, FinishedCalled, ErrorCalled } m_state;
> d->m_state would be set to InsideMethod before calling get(),
> and in both finished() you would say
> if (d->m_state == FinishedCalled) {
> kWarning() << "finished() called twice!";
> } else if (d->m_state == ErrorCalled) {
> kWarning() << "finished() called after error()!";
> }
> and similar in error().
> And when get() returns, you set the state to Idle again. You could
> even check
> that the state was FinishedCalled or ErrorCalled.
>
> The point is that get() (and all other operations) is a blocking
> method call. So there's no
> need for any job ID: you know that it's done when it returns, and
> there can only be one
> operation at a time.
Thank you for this suggestion. I'm attaching a patch where
slavebase.cpp and slavebase.h are modified accordingly, and I've
verified that warnings are printed whenever the kio_smb tries to call
finished() after error().
Unfortunately, I'm having trouble committing the patch due to my ISP's
firewall, so if it looks OK, please feel free to commit it.
Thanks,
Vlad
____________________________________________________________________________________
Be a better friend, newshound, and
know-it-all with Yahoo! Mobile. Try it now. http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ
["slavebase.diff" (text/x-patch)]
Index: kio/kio/slavebase.cpp
===================================================================
--- kio/kio/slavebase.cpp (revision 778642)
+++ kio/kio/slavebase.cpp (working copy)
@@ -102,13 +102,14 @@
KIO::filesize_t sentListEntries;
KRemoteEncoding *remotefile;
time_t timeout;
+ enum { Idle, InsideMethod, FinishedCalled, ErrorCalled } m_state;
QByteArray timeoutData;
// Reconstructs configGroup from configData and mIncomingMetaData
void rebuildConfig()
{
configGroup->deleteGroup(KConfigGroup::WriteConfigFlags());
-
+
// mIncomingMetaData cascades over config, so we write config first,
// to let it be overwritten
MetaData::ConstIterator end = configData.constEnd();
@@ -119,6 +120,13 @@
for (MetaData::ConstIterator it = q->mIncomingMetaData.constBegin(); it != \
end; ++it)
configGroup->writeEntry(it.key(), it->toUtf8(), \
KConfigGroup::WriteConfigFlags()); }
+
+ void verifyState(const QString& cmdName)
+ {
+ if ((m_state != FinishedCalled) && (m_state != ErrorCalled)){
+ kWarning(7019) << cmdName << " did not call finished() or error()! \
Please fix the KIO slave."; + }
+ }
};
}
@@ -386,6 +394,15 @@
void SlaveBase::error( int _errid, const QString &_text )
{
+ if (d->m_state == d->ErrorCalled) {
+ kWarning(7019) << "error() called twice! Please fix the KIO slave.";
+ return;
+ } else if (d->m_state == d->FinishedCalled) {
+ kWarning(7019) << "error() called after finished()! Please fix the KIO \
slave."; + return;
+ }
+
+ d->m_state = d->ErrorCalled;
mIncomingMetaData.clear(); // Clear meta data
d->rebuildConfig();
mOutgoingMetaData.clear();
@@ -406,6 +423,15 @@
void SlaveBase::finished()
{
+ if (d->m_state == d->FinishedCalled) {
+ kWarning(7019) << "finished() called twice! Please fix the KIO slave.";
+ return;
+ } else if (d->m_state == d->ErrorCalled) {
+ kWarning(7019) << "finished() called after error()! Please fix the KIO \
slave."; + return;
+ }
+
+ d->m_state = d->FinishedCalled;
mIncomingMetaData.clear(); // Clear meta data
d->rebuildConfig();
sendMetaData();
@@ -899,7 +925,7 @@
}
else
{
- kWarning() << "Got cmd " << cmd << " while waiting for an answer!";
+ kWarning(7019) << "Got cmd " << cmd << " while waiting for an answer!";
}
}
}
@@ -939,20 +965,24 @@
QString host, user;
quint16 port;
stream >> host >> port >> user >> passwd;
+ d->m_state = d->InsideMethod;
setHost( host, port, user, passwd );
- }
- break;
- case CMD_CONNECT:
+ d->verifyState(QString("setHost()"));
+ d->m_state = d->Idle;
+ } break;
+ case CMD_CONNECT: {
openConnection( );
- break;
- case CMD_DISCONNECT:
+ } break;
+ case CMD_DISCONNECT: {
closeConnection( );
- break;
- case CMD_SLAVE_STATUS:
+ } break;
+ case CMD_SLAVE_STATUS: {
+ d->m_state = d->InsideMethod;
slave_status();
- break;
- case CMD_SLAVE_CONNECT:
- {
+ d->verifyState(QString("slave_status()"));
+ d->m_state = d->Idle;
+ } break;
+ case CMD_SLAVE_CONNECT: {
d->onHold = false;
QString app_socket;
QDataStream stream( data );
@@ -962,8 +992,7 @@
d->isConnectedToApp = true;
connectSlave(app_socket);
} break;
- case CMD_SLAVE_HOLD:
- {
+ case CMD_SLAVE_HOLD: {
KUrl url;
QDataStream stream( data );
stream >> url;
@@ -974,9 +1003,12 @@
// Do not close connection!
connectSlave(d->poolSocket);
} break;
- case CMD_REPARSECONFIGURATION:
+ case CMD_REPARSECONFIGURATION: {
+ d->m_state = d->InsideMethod;
reparseConfiguration();
- break;
+ d->verifyState(QString("reparseConfiguration()"));
+ d->m_state = d->Idle;
+ } break;
case CMD_CONFIG: {
stream >> d->configData;
d->rebuildConfig();
@@ -985,21 +1017,23 @@
#endif
delete d->remotefile;
d->remotefile = 0;
- break;
- }
- case CMD_GET:
- {
+ } break;
+ case CMD_GET: {
stream >> url;
+ d->m_state = d->InsideMethod;
get( url );
+ d->verifyState(QString("get()"));
+ d->m_state = d->Idle;
} break;
- case CMD_OPEN:
- {
+ case CMD_OPEN: {
stream >> url >> i;
QIODevice::OpenMode mode = QFlag(i);
+ d->m_state = d->InsideMethod;
open(url, mode);
+ d->verifyState(QString("open()"));
+ d->m_state = d->Idle;
} break;
- case CMD_PUT:
- {
+ case CMD_PUT: {
int permissions;
qint8 iOverwrite, iResume;
stream >> url >> iOverwrite >> iResume >> permissions;
@@ -1012,98 +1046,135 @@
// (the resume bool is currently unused)
d->needSendCanResume = true /* !resume */;
+ d->m_state = d->InsideMethod;
put( url, permissions, flags);
+ d->verifyState(QString("put()"));
+ d->m_state = d->Idle;
} break;
- case CMD_STAT:
+ case CMD_STAT: {
stream >> url;
+ d->m_state = d->InsideMethod;
stat( url );
- break;
- case CMD_MIMETYPE:
+ d->verifyState(QString("stat()"));
+ d->m_state = d->Idle;
+ } break;
+ case CMD_MIMETYPE: {
stream >> url;
+ d->m_state = d->InsideMethod;
mimetype( url );
- break;
- case CMD_LISTDIR:
+ d->verifyState(QString("mimetype()"));
+ d->m_state = d->Idle;
+ } break;
+ case CMD_LISTDIR: {
stream >> url;
+ d->m_state = d->InsideMethod;
listDir( url );
- break;
- case CMD_MKDIR:
+ d->verifyState(QString("listDir()"));
+ d->m_state = d->Idle;
+ } break;
+ case CMD_MKDIR: {
stream >> url >> i;
+ d->m_state = d->InsideMethod;
mkdir( url, i );
- break;
- case CMD_RENAME:
- {
+ d->verifyState(QString("mkdir()"));
+ d->m_state = d->Idle;
+ } break;
+ case CMD_RENAME: {
qint8 iOverwrite;
KUrl url2;
stream >> url >> url2 >> iOverwrite;
JobFlags flags;
if ( iOverwrite != 0 ) flags |= Overwrite;
+ d->m_state = d->InsideMethod;
rename( url, url2, flags );
+ d->verifyState(QString("rename()"));
+ d->m_state = d->Idle;
} break;
- case CMD_SYMLINK:
- {
+ case CMD_SYMLINK: {
qint8 iOverwrite;
QString target;
stream >> target >> url >> iOverwrite;
JobFlags flags;
if ( iOverwrite != 0 ) flags |= Overwrite;
+ d->m_state = d->InsideMethod;
symlink( target, url, flags );
+ d->verifyState(QString("symlink()"));
+ d->m_state = d->Idle;
} break;
- case CMD_COPY:
- {
+ case CMD_COPY: {
int permissions;
qint8 iOverwrite;
KUrl url2;
stream >> url >> url2 >> permissions >> iOverwrite;
JobFlags flags;
if ( iOverwrite != 0 ) flags |= Overwrite;
+ d->m_state = d->InsideMethod;
copy( url, url2, permissions, flags );
+ d->verifyState(QString("copy()"));
+ d->m_state = d->Idle;
} break;
- case CMD_DEL:
- {
+ case CMD_DEL: {
qint8 isFile;
stream >> url >> isFile;
+ d->m_state = d->InsideMethod;
del( url, isFile != 0);
+ d->verifyState(QString("del()"));
+ d->m_state = d->Idle;
} break;
- case CMD_CHMOD:
+ case CMD_CHMOD: {
stream >> url >> i;
+ d->m_state = d->InsideMethod;
chmod( url, i);
- break;
- case CMD_CHOWN:
- {
+ d->verifyState(QString("chmod()"));
+ d->m_state = d->Idle;
+ } break;
+ case CMD_CHOWN: {
QString owner, group;
stream >> url >> owner >> group;
+ d->m_state = d->InsideMethod;
chown(url, owner, group);
- break;
- }
- case CMD_SETMODIFICATIONTIME:
- {
+ d->verifyState(QString("chown()"));
+ d->m_state = d->Idle;
+ } break;
+ case CMD_SETMODIFICATIONTIME: {
QDateTime dt;
stream >> url >> dt;
+ d->m_state = d->InsideMethod;
setModificationTime(url, dt);
+ d->verifyState(QString("setModificationTime()"));
+ d->m_state = d->Idle;
} break;
- case CMD_SPECIAL:
+ case CMD_SPECIAL: {
+ d->m_state = d->InsideMethod;
special( data );
- break;
+ d->verifyState(QString("special()"));
+ d->m_state = d->Idle;
+ } break;
case CMD_META_DATA: {
//kDebug(7019) << "(" << getpid() << ") Incoming meta-data...";
stream >> mIncomingMetaData;
d->rebuildConfig();
- break;
- }
- case CMD_SUBURL:
+ } break;
+ case CMD_SUBURL: {
stream >> url;
+ d->m_state = d->InsideMethod;
setSubUrl(url);
- break;
- case CMD_NONE:
- fprintf(stderr, "Got unexpected CMD_NONE!\n");
- break;
- case CMD_MULTI_GET:
+ d->verifyState(QString("setSubUrl()"));
+ d->m_state = d->Idle;
+ } break;
+ case CMD_NONE: {
+ kWarning(7019) << "Got unexpected CMD_NONE!";
+ } break;
+ case CMD_MULTI_GET: {
+ d->m_state = d->InsideMethod;
multiGet( data );
- break;
- default:
+ d->verifyState(QString("multiGet()"));
+ d->m_state = d->Idle;
+ } break;
+ default: {
// Some command we don't understand.
// Just ignore it, it may come from some future version of KDE.
- break;
+ } break;
}
}
Index: kio/kio/slavebase.h
===================================================================
--- kio/kio/slavebase.h (revision 778642)
+++ kio/kio/slavebase.h (working copy)
@@ -91,7 +91,8 @@
/**
* Call to signal an error.
- * This also finishes the job, no need to call finished.
+ * This also finishes the job, so you must not call
+ * finished() after calling this.
*
* If the Error code is KIO::ERR_SLAVE_DEFINED then the
* _text should contain the complete translated text of
@@ -115,7 +116,8 @@
/**
* Call to signal successful completion of any command
- * (besides openConnection and closeConnection)
+ * besides openConnection and closeConnection. Do not
+ * call this after calling error().
*/
void finished();
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic