Add ability to pull in carddav contacts

[jabjoe]

All the contacts I want to call are already in carddav databases, so I just needed to connect Baresip to that.

[juha-h]

How about adding and removing contacts to/from carddav database?

[jabjoe]

I've not seen a decent contacts editor for Baresip, so I figured it was a non-issue. Personally my contacts are edited on my phone, Thunderbird or Nextcloud.

Also the contacts in Baresip is just name and sip. So you loose information if you edit it there.

What it could do I guess is push up contacts in Baresip and not the carddav.

[juha-h]

I was just thinking that if baresip receives call from someone what is not in contacts, there could be command to add it (name given by baresip user and SIP URI from the INVITE) or delete the SIP URI from existing contact.

In baresip Android app I don't use baresip contact module. In the app user can select whether to use baresip app's contacts, Android contacts or both and contacts can be added/removed to/from either contacts. carddav could be third option, but it does not make sense without add/remove possibility that already exists in the previous two.

[cspiel1]

Please could you rebase and resolve the comments?

[cspiel1]

Please could you rebase and resolve the comments?

Still to do.

[jabjoe]

This still work WIP due to the regex changes. Should be done in a few days. Those commits where just broken out and pushed as they weren't regex related. I'll stop pushing until it's done now.

[jabjoe]

RE's Regex version pushed.

[cspiel1]

You should follow https://www.rfc-editor.org/rfc/rfc6350.html#section-3 when parsing vCard format.

• https://curl.se/libcurl/c/CURLOPT_WRITEFUNCTION.html tells me that the received data has to be collected. Here ideally use mbuf!
• Then un-fold logical lines! (https://www.rfc-editor.org/rfc/rfc6350.html#section-3.2)
• Parse line-by-line (logical lines)!
• A vCard always looks like this:

vcard = "BEGIN:VCARD" CRLF
        "VERSION:4.0" CRLF
        1*contentline
        "END:VCARD" CRLF

• See: https://www.rfc-editor.org/rfc/rfc6350.html#section-3.3. Then re_regex() is helpful. Can we rename the struct to shorter struct carddav?

static int process_line(struct carddav *carddav,
							   const struct pl *line)
{
	struct pl group      = PL_INIT;
	struct pl name_param = PL_INIT;
	struct pl name       = PL_INIT;
	struct pl params     = PL_INIT;
	struct pl value      = PL_INIT;
	int err;

	/* group present */
	err = re_regex(line->p, line->l, "[~\\.]+\\.[~:]*:[~]*",
		           &group, &name_param, &value);
	if (err) {
		/* without group */
		group = pl_null;
		err = re_regex(line->p, line->l, "[~:]*:[~]*",
			           &name_param, &value);
	}
	if (err) {
		warning("invalid contentline: %r\n", line);
		return EINVAL;
	}

	err = process_name_param(&name_param, &name, &params);
	if (err)
		return err;

	/* Process decoded data  (replace the re_printf here). If it is needed, store in carddav object.
	   - Ignore everything outside BEGIN:VCARD and END:VCARD!
	   - Check that VERSION follows BEGIN:VCARD!
	   - Add a contact if found a SIP line.
	 */
	re_printf("group:  %r\n", &group);
	re_printf("name:   %r\n", &name);
	re_printf("params: %r\n", &params);
	re_printf("value:  %r\n", &value);
	return 0;
}

And then split name and params in a separate function, because params are optional:

static int process_name_param(const struct pl *name_param,
							  struct pl *name,
							  struct pl *params)
{
	if (!pl_isset(name_param))
		return EINVAL;

	/* parameter present */
	int err = re_regex(name_param->p, name_param->l, "[~;]*;[~:]*",
			           name, params);
	if (err)
		*name = *name_param;

	return 0;
}

[jabjoe]

Before I go further, is this use of mbuf acceptable?

jabjoe@d88be03

https://github.com/jabjoe/baresip-apps/blob/dev_carddav_contacts_mbuf_rebase/modules/carddav/carddav.c

I've also ditched the use of those buffers elsewhere and just used stack memory big enough to cover even big lengths for the use.

I've stuck with double buffer bit as I don't like idea of one big buffer for all the response. Seams like giving an open checkbook to ram consumption and just isn't needed as no single carddav is that big. (Even with a embedded photo.)

If that ok, I'll stick this commit in the main dev branch and start looking again at the carddav processing.

[cspiel1]

A continuous processing while downloading would be beneficial. What about:

• Download one bunch.
• Process the bunch
  • concatenate physical lines until you get one logical line

process a bunch

static size_t writefunc(const void *ptr,
                        size_t size,
                        size_t nmemb,
                        void *userdata)
{
	struct carddav *d = userdata;
	size_t total = size*nmemb;
	int err;

	debug("carddav: Chunk of %zu\n", total);
	const char *mbend = (const char *) mbuf_buf(d->mb);
	const char *begin = "BEGIN:VCARD\r\n";
	const char *end   = "END:VCARD\r\n";

	if (!d->bpos) {
		d->bpos = mbend;
		d->epos = mbend;
	}

	err = mbuf_write_mem(d->mb, ptr, total);
	if (err)
		return 0;

	/* 1. TODO unfold: remove "/r/n " (CR LF SPACE)
	 *  start at (mbend - 3) to look for CR LF SPACE                 */

	/* 2. look for BEGIN:VCARD, suggested like here */

	mbend = (const char *) mbuf_buf(d->mb);
	struct pl bcheck  = {d->bpos, mbend - d->bpos};
	struct pl echeck  = {d->epos, mbend - d->epos};
	const char *pos = NULL;
	if (!d->vcard && (pos = pl_strstr(&bcheck, begin))) {
		debug("carddav: Found card start.\n");
		pos += strlen(begin);
		d->vcard = pos;
		d->epos  = pos;
	}
	else if (d->vcard && (pos = pl_strstr(&echeck, end))) {
		/* 3. look for END:VCARD and rewind mbuf to save memory */
		debug("carddav: Found card end.\n");
		char *tail = NULL;

		pos += strlen(end);
		size_t tail_len = mbend - pos;
		if (tail_len > 0) {
			mbuf_set_pos(d->mb, mbuf_pos(d->mb) - tail_len);
			err = mbuf_strdup(d->mb, &tail, tail_len);
			if (err)
				return 0;
		}

		struct pl card = {d->vcard, d->vcard - pos};
		err = process_vcard(d, &card);
		mbuf_rewind(d->mb);
		d->vcard = NULL;
		d->bpos  = NULL;

		if (tail) {
			mbuf_write_str(d->mb, tail);
			mem_deref(tail);
		}
	}

	return total;
}

process found vcard line by line

static int process_vcard(struct carddav *d, const struct pl *card)
{
	if (!pl_isset(card))
		return EINVAL;

	const char *pos = card->p;
	const char *end = card->p + card->l;

	while (pos < end) {
		struct pl tail = {pos, end - pos};
		const char *lineend = pl_strstr(&tail, "\r\n");
		if (!lineend)
			break;

		struct pl line = {pos, lineend - pos};
		int err;
		err = process_line(d, &line);
		if (err)
			return err;

		pos = line.p + line.l + 2;
	}

	return 0;
}

• process_line() - process the logical line (like stated here: Add ability to pull in carddav contacts #102 (comment))
  • check if VERSION is the first line, which is mandatory due to RFC
  • find SIP lines

Note, I did not compile or test this. Could you give it a try?

[cspiel1]

Please put the upload part into a separate PR or draft! Thus please drop the upload part in this PR for now!

Here I have a branch starting from yours which compiles. Where could I get test data?
https://github.com/cspiel1/baresip-apps/tree/carddav_cspiel

[jabjoe]

The problem I have with this (very neat) code is:

• Unlimited amount of allocation before start of vcard. I feel we need some kind of limiting. Which in my case was the size of the A/B buffers.

• It doesn't account for cards being split between blocks, which does happen. You have to start at the beginning of the buffer each time, not the current chunk.

• I don't like all tail allocation/deallocation. That's why I did the A/B buffer thing. Just copy the tail to start of the next buffer and make that the new primary.

The "process_vcard/process_line" I'm happy to work with. It is neater than what I hacked together.

[cspiel1]

The problem I have with this (very neat) code is:

• Unlimited amount of allocation before start of vcard. I feel we need some kind of limiting. Which in my case was the size of the A/B buffers.

• mbuf_alloc(x) is only an initial amount of memory. It internally grows and you can limit this from outside by checking e.g. mbuf_end(). Also it will grow only to the size of the maximum vCard. We don't need to download the whole file first.

• It doesn't account for cards being split between blocks, which does happen. You have to start at the beginning of the buffer each time, not the current chunk.

• It does. The writefunc() collects blocks until the end of the current card is found. Then the card is processed.

• I don't like all tail allocation/deallocation. That's why I did the A/B buffer thing. Just copy the tail to start of the next buffer and make that the new primary.

• mbuf() can help here a lot. You don't have to allocate a lot of memory beforehand or use a big stack buffer.

The "process_vcard/process_line" I'm happy to work with. It is neater than what I hacked together.

Thanks! Please give it a try!

[jabjoe]

The problem I have with this (very neat) code is:

• Unlimited amount of allocation before start of vcard. I feel we need some kind of limiting. Which in my case was the size of the A/B buffers.

* `mbuf_alloc(x)` is only an initial amount of memory. It internally grows and you can limit this from outside by checking e.g. `mbuf_end()`. Also it will grow only to the size of the maximum vCard. We don't need to download the whole file first.

That's a way of limiting the growth. My preference is have fixed size allocation to start with.

• It doesn't account for cards being split between blocks, which does happen. You have to start at the beginning of the buffer each time, not the current chunk.

* It does. The `writefunc()` collects blocks until the end of the current card is found. Then the card is processed.

It's not just end of the card, but the beginning. You can get the beginning split over a boundary. So you need to start at the absolute start of the buffer after you added the next chunk.

So instead of:

const char *mbend = (const char *) mbuf_buf(d->mb);

It should be:

const char *mbend = (const char *) d->mb->buf;

• I don't like all tail allocation/deallocation. That's why I did the A/B buffer thing. Just copy the tail to start of the next buffer and make that the new primary.

* `mbuf()` can help here a lot. You don't have to allocate a lot of memory beforehand or use a big stack buffer.

But if you are going to end up using a chunk anyway, I feel it is neater to allocate it upfront instead of keeping on allocating and deallocating more and more. Stick in a default large enough to cover most cases, and leave it config adjustable if someone has like 4K images in their vcards. With A/B, you don't need all those random size allocation/deallocation for tail. You just use B for the tail and swap.

The "process_vcard/process_line" I'm happy to work with. It is neater than what I hacked together.

Thanks! Please give it a try!

Done and it is neater. :-)

[cspiel1]

Did another review iteration and added some comments. Most of the algorithm I'd accept now.

My feeling is that the mbuf A-B swapping is not needed. But if you think, then I'd also accept it for now.

Is I wrote in one comment, please avoid large static buffers and use allocation and mem_unref() if possible!

• E.g. char addr[1024];

Note: I'll be on vacation until Monday next week.

[cspiel1]

Thanks for your changes! I am on vacation now for a week. Then I'll have a further details look.

[cspiel1]

Dear @jabjoe, I added several comments. Please process them or at least comment each!

Thanks for your contribution! It already improved a lot. But about the usage of mbuf I don't like to discuss any longer. For embedded systems there is not always unlimited memory available. So please accept my suggestions for mbuf!

[jabjoe]

Work has been busy and I'm wrapping up another personal project before I get back to this. I will say part of the point of the A/B buffer was to limit allocations and allocation size. Not unlimited memory. Anyway I'll look when I'm free. No one is in a rush here. :-)

[jabjoe]

Hi again, I've finished the other project. Looking again to close this one up. I'm missing something. I can only find 5 comments, but they are all old and marked as resolved.

I'm going to:

https://github.com/baresip/baresip-apps/pull/102/changes

Then the triple dots menu and selecting comments.
Can't see I'd done anything to nuke them as this was the first time I went to look for this batch.
I can you point me in the right direction?
Let's get this closed up. 😃

[cspiel1]

Not sure which one are open. I have to look again. Most likely this week.

[jabjoe]

Cheers. :-)