On Wednesday 19 of September 2012 22:11:28 Paul Menzel wrote:
> Am Mittwoch, den 19.09.2012, 12:29 +0200 schrieb Dan Vratil:
> > On Wednesday 19 of September 2012 11:20:26 Paul Menzel wrote:
>
> > On Wednesday 19 of September 2012 11:20:26 Paul Menzel wrote:
> ^^
>
> Is putting the line introducing the quote twice a bug in a KMail
> version?
No, that's my incapability to correctly copy&paste plain text :)
>
> > > For whatever reasons, addressbooks can be corrupt. Clicking on such an
> > > entry in the Contacts overview crashes Evolution with a segmentation
> > > fault. In the overview the first line of the excerpt is empty.
> > >
> > > Therefore the return values have to be checked, which is also good
> > > programming practice.
> >
> > <snip>
> >
> > > ---
> > > What should be done in `e_destination_set_contact()` in the error case?
> > > A warning to the user would be nice telling her/him to edit the
> > > addressbook or Evolution should fix up the addressbook entry itself but
> > > also notifying the user.
> > >
> > > addressbook/libebook/e-destination.c | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/addressbook/libebook/e-destination.c
> > > b/addressbook/libebook/e-destination.c index 96582d1..9808fe8 100644
> > > --- a/addressbook/libebook/e-destination.c
> > > +++ b/addressbook/libebook/e-destination.c
> > > @@ -463,8 +463,13 @@ e_destination_set_contact (EDestination *dest,
> > >
> > > raw = e_vcard_attribute_get_value (attr->data);
> > > addr = camel_internet_address_new ();
> > >
> > > - camel_address_unformat (CAMEL_ADDRESS (addr),
raw);
> > > - camel_internet_address_get (addr, 0, &name,
&email);
> > > + if (camel_address_unformat (CAMEL_ADDRESS (addr),
raw) <= 0) {
> > > + /* Report an error that contact is corrupt */
> > > + }
> > > +
> > > + if (!camel_internet_address_get (addr, 0, &name,
&email)) {
> > > + /* Report an error corrupt */
> > > + }
> > >
> > > e_destination_set_name (s_dest, name);
> > > e_destination_set_email (s_dest, email);
> >
> > In general I agree with your approach, but I don't know whether it makes
> > sense to report error to user here (can there be other reasons for
> > camel_internet_address_get() to fail other then addressbook corruption?).
>
> Unfortunately I do not know.
>
> > I'd simply suggest something like:
> >
> > if (camel_address_unformat (CAMEL_ADDRESS (addr), raw) > 0) {
> >
> > if (camel_internet_address_get (addr, 0, &name, &email)) {
> >
> > e_destination_set_name (s_dest, name);
> > e_destination_set_email (s_dest, email);
> >
> > }
> >
> > }
>
> Still the user should be warned somehow and told how to fix this.
Hmm, you could probably update e_destination_set_contact() to return FALSE
upon failure, then check for the return value in EABContactFormatter. The
async operation running in formatter can set a GError value and terminate. The
GError can be picked by EABContactDisplay which can finally display the error
to user (using EAlertSink).
Thinking about this I remembered that Milan told me some time ago that it
would be nice to make the new formatters and parsers to support GErrors (he
was talking about the mail formatter, but the addressbook is using the same
concept). This would give us some nice error reporting. So far when the
formatter fails, you either get an empty preview pane or "Failed to load part
blahblahblah" message. Doing this in the addressbook formatter might be a nice
"research" on how it should look and work :-)
Cheers,
Dan
>
> > I think the _actual_ problem is, that name and email variables are not
> > initialized before being passed to camel_internet_address_get(). The crash
> > comes from g_strdup() in e_destination_set_{name,email}() which tries to
> > duplicate an invalid memory. According to docs, g_strdup(NULL) returns
> > NULL, so that would prevent any crash in the first place. But as you said
> > above, checking return value is a good practice, so combining these two
> > would be valid solution.
>
> Interesting. I will try to take a look.
>
> > [ Off-topic: could we consider enforcing stricter compiler warnings?
> > Getting "possible use of uninitialized value" and similar warnings would
> > for prevent us from introducing crashes like this one. ]
>
> Good idea.
>
>
> Thanks,
>
> Paul
--
dvratil redhat com | Associate Software Engineer / BaseOS / KDE, Qt
GPG Key: 0xC59D614F6F4AE348
Fingerprint: 4EC1 86E3 C54E 0B39 5FDD B5FB C59D 614F 6F4A E348Attachment:
signature.asc
Description: This is a digitally signed message part.