Page MenuHomeFreeBSD
Differential D29188

net/kamailio: Update to 5.4.4
ClosedPublic
Actions

Authored by lcook on Mar 10 2021, 3:22 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 16, 8:59 AM
Unknown Object (File)
Mar 23 2025, 8:25 AM
Unknown Object (File)
Mar 5 2025, 6:43 PM
Unknown Object (File)
Feb 24 2025, 6:02 PM
Unknown Object (File)
Feb 23 2025, 6:01 PM
Unknown Object (File)
Jan 9 2025, 4:54 AM
Unknown Object (File)
Jan 4 2025, 12:51 AM
Unknown Object (File)
Jan 4 2025, 12:48 AM
View All 88 Files
Subscribers
mat

Details

Reviewers
tcberner
fernape
Commits
rP568045: net/kamailio: Update to 5.4.4
Summary

While here, appease some of the suggestions by portclippy/portfmt.
Changes: https://www.kamailio.org/pub/kamailio/5.4.4/ChangeLog

Test Plan

QA:

  • portlint: OK (1 WARN).
  • testport: OK (poudriere: 12.2-RELEASE-p4, amd64/i386).

PR: 254187
Submitted by: Dmitry Wagin <dmitry.wagin@ya.ru> (maintainer)

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 37733
Build 34622: arc lint + arc unit

Event Timeline

lcook created this revision.Mar 10 2021, 3:22 PM
Herald added a subscriber: mat. · View Herald TranscriptMar 10 2021, 3:22 PM
lcook requested review of this revision.Mar 10 2021, 3:22 PM
Harbormaster completed remote builds in B37729: Diff 85484.Mar 10 2021, 3:22 PM
fernape added a comment.Mar 10 2021, 3:29 PM

This port would benefit from some cleanup. It would be better if variables were sorted, especially the option helpers.
When I find things like this, I usually report back to the submitter/maintainer in the PR or I fixed them myself.

Sometimes I sound like Marie Kondo, but... :-)

lcook added a comment.Mar 10 2021, 3:35 PM
In D29188#653433, @fernape wrote:

This port would benefit from some cleanup. It would be better if variables were sorted, especially the options helpers.
When I find things like this, I usually report back to the submitter/maintainer in the PR or I fixed them myself.

Sometimes I sound like Marie Kondo, but... :-)

I thought about ordering the varying OPTIONS too, just didn't know at what point is considered "too much" even though we retain the right to make these changes as we see fit. Though you're right; best to embrace the tidy-up. I'll get to it ;)

fernape added a comment.Mar 10 2021, 3:43 PM
In D29188#653434, @lcook wrote:
In D29188#653433, @fernape wrote:

This port would benefit from some cleanup. It would be better if variables were sorted, especially the options helpers.
When I find things like this, I usually report back to the submitter/maintainer in the PR or I fixed them myself.

Sometimes I sound like Marie Kondo, but... :-)

I thought about ordering the varying OPTIONS too, just didn't know at what point is considered "too much" even though we retain the right to make these changes as we see fit. Though you're right; best to embrace the tidy-up. I'll get to it ;)

Thanks!

I think it is wise to friendly advise the submitter/maintainer. Sometimes I do it before the commit and sometimes I ask them if they would do the changes themselves
Most people are really receptive to suggestions :-)

lcook updated this revision to Diff 85490.Mar 10 2021, 3:47 PM

Tidy-up the options helpers and sort alphabetically

Harbormaster completed remote builds in B37733: Diff 85490.Mar 10 2021, 3:47 PM
lcook added a comment.Mar 10 2021, 3:50 PM
In D29188#653481, @lcook wrote:

Tidy-up the options helpers and sort alphabetically

I'll attach this in the bug-report too, just to make sure they're happy with the changes.

lcook added a comment.Mar 10 2021, 5:03 PM

Right it appears the maintainer is OK with the changes. Can we ship it? :)

fernape accepted this revision.Mar 10 2021, 9:16 PM
In D29188#653517, @lcook wrote:

Right it appears the maintainer is OK with the changes. Can we ship it? :)

Sure we can!

This revision is now accepted and ready to land.Mar 10 2021, 9:16 PM
Closed by commit rP568045: net/kamailio: Update to 5.4.4 (authored by lcook). · Explain WhyMar 10 2021, 9:25 PM
This revision was automatically updated to reflect the committed changes.
lcook added a commit: rP568045: net/kamailio: Update to 5.4.4.

Revision Contents
Changeset List

PathSize
net/
kamailio/
101 lines
6 lines

Diff 85490

View Options

net/kamailio/Makefile

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines for (n = 0; n < G_N_EVENTREFS; n++) {
break; break;
} }
} }
if (TAILQ_EMPTY(&g_events)) if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events); wakeup(&g_pending_events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
} }
static int struct g_event *
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap) g_alloc_event(int flag)
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
impAuthorUnsubmitted
Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

That's a good idea.

imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of g_vfs_done:

	if (destroy)
		g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL);

That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?)

Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted.

jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{ {
struct g_event *ep; KASSERT(flag == M_WAITOK || flag == M_NOWAIT,
("Wrong flag to g_alloc_event"));
return (g_malloc(sizeof(struct g_event), flag | M_ZERO));
}
static void
g_post_event_ep_va(g_event_t *func, void *arg, int wuflag,
struct g_event *ep, va_list ap)
{
void *p; void *p;
u_int n; u_int n;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag | M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag; ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) { for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *); p = va_arg(ap, void *);
if (p == NULL) if (p == NULL)
break; break;
g_trace(G_T_TOPOLOGY, " ref %p", p); g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p; ep->ref[n] = p;
} }
KASSERT(p == NULL, ("Too many references to event")); KASSERT(p == NULL, ("Too many references to event"));
ep->func = func; ep->func = func;
ep->arg = arg; ep->arg = arg;
mtx_lock(&g_eventlock); mtx_lock(&g_eventlock);
TSHOLD("GEOM events"); TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events); TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
wakeup(&g_wait_event); wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags |= TDP_GEOM; curthread->td_pflags |= TDP_GEOM;
thread_lock(curthread); thread_lock(curthread);
curthread->td_flags |= TDF_ASTPENDING; curthread->td_flags |= TDF_ASTPENDING;
thread_unlock(curthread); thread_unlock(curthread);
}
void
g_post_event_ep(g_event_t *func, void *arg, struct g_event *ep, ...)
jhbUnsubmitted
Not Done
Inline Actions

Why did you expose wuflag in this API? I would perhaps instead just have a new g_waitfor_event_storage if someone needs the EV_WAKEUP variant so that it mirrors the g_post_event/g_waitfor_event API. The 'waitfor' variant could be added in the future when it is needed.

Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use M_WAITOK to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses M_WAITOK.

jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
impAuthorUnsubmitted
Done
Inline Actions

I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense.

imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
{
va_list ap;
va_start(ap, ep);
g_post_event_ep_va(func, arg, 0, ep, ap);
va_end(ap);
}
static int
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap)
{
struct g_event *ep;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_alloc_event(flag);
if (ep == NULL)
return (ENOMEM);
if (epp != NULL)
*epp = ep;
g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0); return (0);
} }
int int
g_post_event(g_event_t *func, void *arg, int flag, ...) g_post_event(g_event_t *func, void *arg, int flag, ...)
{ {
va_list ap; va_list ap;
int i; int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines
View Options

net/kamailio/distinfo

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Lines for (n = 0; n < G_N_EVENTREFS; n++) {
break; break;
} }
} }
if (TAILQ_EMPTY(&g_events)) if (TAILQ_EMPTY(&g_events))
wakeup(&g_pending_events); wakeup(&g_pending_events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
} }
static int struct g_event *
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap) g_alloc_event(int flag)
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

jhb: Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
impAuthorUnsubmitted
Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

That's a good idea.

imp: > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
jhbUnsubmitted
Not Done
Inline Actions

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of g_vfs_done:

	if (destroy)
		g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL);

That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?)

Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted.

jhb: > > Do we need a `g_free_event` as well? In your case in geom_disk you are going to consume it…
{ {
struct g_event *ep; KASSERT(flag == M_WAITOK || flag == M_NOWAIT,
("Wrong flag to g_alloc_event"));
return (g_malloc(sizeof(struct g_event), flag | M_ZERO));
}
static void
g_post_event_ep_va(g_event_t *func, void *arg, int wuflag,
struct g_event *ep, va_list ap)
{
void *p; void *p;
u_int n; u_int n;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_malloc(sizeof *ep, flag | M_ZERO);
if (ep == NULL)
return (ENOMEM);
ep->flag = wuflag; ep->flag = wuflag;
for (n = 0; n < G_N_EVENTREFS; n++) { for (n = 0; n < G_N_EVENTREFS; n++) {
p = va_arg(ap, void *); p = va_arg(ap, void *);
if (p == NULL) if (p == NULL)
break; break;
g_trace(G_T_TOPOLOGY, " ref %p", p); g_trace(G_T_TOPOLOGY, " ref %p", p);
ep->ref[n] = p; ep->ref[n] = p;
} }
KASSERT(p == NULL, ("Too many references to event")); KASSERT(p == NULL, ("Too many references to event"));
ep->func = func; ep->func = func;
ep->arg = arg; ep->arg = arg;
mtx_lock(&g_eventlock); mtx_lock(&g_eventlock);
TSHOLD("GEOM events"); TSHOLD("GEOM events");
TAILQ_INSERT_TAIL(&g_events, ep, events); TAILQ_INSERT_TAIL(&g_events, ep, events);
mtx_unlock(&g_eventlock); mtx_unlock(&g_eventlock);
wakeup(&g_wait_event); wakeup(&g_wait_event);
if (epp != NULL)
*epp = ep;
curthread->td_pflags |= TDP_GEOM; curthread->td_pflags |= TDP_GEOM;
thread_lock(curthread); thread_lock(curthread);
curthread->td_flags |= TDF_ASTPENDING; curthread->td_flags |= TDF_ASTPENDING;
thread_unlock(curthread); thread_unlock(curthread);
}
void
g_post_event_ep(g_event_t *func, void *arg, struct g_event *ep, ...)
jhbUnsubmitted
Not Done
Inline Actions

Why did you expose wuflag in this API? I would perhaps instead just have a new g_waitfor_event_storage if someone needs the EV_WAKEUP variant so that it mirrors the g_post_event/g_waitfor_event API. The 'waitfor' variant could be added in the future when it is needed.

Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use M_WAITOK to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses M_WAITOK.

jhb: Why did you expose `wuflag` in this API? I would perhaps instead just have a new…
impAuthorUnsubmitted
Done
Inline Actions

I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense.

imp: I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes…
{
va_list ap;
va_start(ap, ep);
g_post_event_ep_va(func, arg, 0, ep, ap);
va_end(ap);
}
static int
g_post_event_x(g_event_t *func, void *arg, int flag, int wuflag, struct g_event **epp, va_list ap)
{
struct g_event *ep;
g_trace(G_T_TOPOLOGY, "g_post_event_x(%p, %p, %d, %d)",
func, arg, flag, wuflag);
KASSERT(wuflag == 0 || wuflag == EV_WAKEUP,
("Wrong wuflag in g_post_event_x(0x%x)", wuflag));
ep = g_alloc_event(flag);
if (ep == NULL)
return (ENOMEM);
if (epp != NULL)
*epp = ep;
g_post_event_ep_va(func, arg, wuflag, ep, ap);
return (0); return (0);
} }
int int
g_post_event(g_event_t *func, void *arg, int flag, ...) g_post_event(g_event_t *func, void *arg, int flag, ...)
{ {
va_list ap; va_list ap;
int i; int i;
▲ Show 20 Lines • Show All 59 Lines • Show Last 20 Lines