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

[Kevin Krammer]

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

It is empty/invalid but it is fully initialized.

> > > 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.

Cheers,
Kevin

-- 
Kevin Krammer, KDE developer, xdg-utils developer
KDE user support, developer mentoring
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 190 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20111004/295e3549/attachment.sig>
-------------- next part --------------
_______________________________________________
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/