[Kde-pim] Review Request: avoid the use of an undefined collection

[Guy Maurel]

Hello Kevin!

Thanks for the explanations. Very fine!
On Tuesday, October 04, 2011 07:10:33 PM Kevin Krammer wrote:
> Hi Guy,
> 
> On Tuesday, 2011-10-04, Guy Maurel wrote:
> > Hello Kevin!
> > 
> > I'll try again to precise what I'm meaning...
> > 
> > On Monday, October 03, 2011 08:12:52 PM Kevin Krammer wrote:
> > > On Monday, 2011-10-03, Guy Maurel wrote:
> 
> > > > The current code is not correct, at the place I noticed.
> > > 
> > > The code in DefaultResourceJobPrivate looks correct.
> > 
> > NO, is my answer! Not the part at
> >   DefaultResourceJobPrivate::collectionFetchResult( KJob *job )
> > 
> > Even if one doesn't know what the code is doing, it is NOT correct.
> > One may not use a variable which has not been assigned *before*.
> 
> Right. Access to uninitialized variables is wrong.
> 
> > > Do you mean the place from where it is called?
> > 
> > I mean the lines
> >   foreach ( const Collection &collection, collections ) {
> >     ...
> > and
> >  if(  !resourceCollection.isValid() ) {
> > because the loop is not entered, the variable resourceCollection is NOT
> > set. We don't know what are the values of the memory. The code uses these
> > values of resourceCollection to make a call "isValid". I think, the result
> > is NOT defined, and might be different from one computer to another.
> 
> Ah.
> This is not a problem. resourceCollection is initialized as a null collection 
> through its default (parameter less) constructor.
> 
> http://api.kde.org/4.x-api/kdepimlibs-
> apidocs/akonadi/html/classAkonadi_1_1Collection.html#aaae7b07d6f719471851af986c03d1b12
right, I oversee this.
 
> It is empty/invalid but it is fully initialized.
OK
 
> > > > With my proposal, the agent doesn't crash any more. It might be not
> > > > suffisant enough, but I need some help from the coding guy.
> > > 
> > > Sure, which is why I try to understand the problem :)
> > 
> > I think that the problem isn't to know from where the method is called
> > from. More, the question is (I hope you could give me an answer):
> > WHAT is to do when the job has NO collection?
> > This is the fact I observe, and the line
> >   kDebug() << "Fetched" << collections.count() << "collections.";
> > informs us about that.
> > 
> > Looking once more on the code I would to ask:
> > What happens in the loop foreach, if the if-statement returns "false"?
> 
> When that if's expression evaluates to "false" then the loop continues until 
> the end.
> resourceCollection remains an empty/invalid collection and toRecover remains 
> an empty list.
> 
> > The lines under
> >   // Find all children of the resource collection.
> > are using the variables resourceCollection and toRecover whitout any check.
> 
> At this point the code has passed the !resourceCollection.isValid() check, 
> meaning the first loop found the collection it was looking for, assigned it to 
> resourceCollection and added it to the toRecover list.
> 
> > Same at
> >   // These collections have been created by the maildir resource, when it
> >   // found the folders on disk. So give them the necessary attributes now.
> > 
> > Nothing take care IF there where NO collection!
> 
> At this point we do have at least one collection in toRecover (the collection 
> assigned to resourceCollection).
> 
> In any case, i.e. even if toRecover would be empty, the check for 
> mPendingModifyJobs == 0 would be true and the resource scan job functionality 
> started.
> 
> > My proposal with "return" might be too short, I know it!
> > What is better?
> 
> I don't think it is necessary.
> The problem that no collection is found is handled by
>   if ( !resourceCollection.isValid() )
> ending the job with an error.
> 
> Your return basically results in the same, just without specific error code and 
> error message.
mmmmh!
The reason why I had a look on this file was the error message I got!
This was  misleading.
If all this is correct (I aggree you now), then we should find anything better,
so that no error message will be generated?
Such as a test:
  bool test= false;
  foreach...{
    if( ... {
      ...
      test= true;
      break;
    }
  }
  ...
   if ( !resourceCollection.isValid() ) {
     if( test) {
       q->setError( Job::Unknown );
       ...
       return;
     
-- 
guy
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/