[MERGED] libosmocore[master]: add osmo_fsm_inst_update_id_f()

[Neels Hofmeyr]

Neels Hofmeyr has submitted this change and it was merged.

Change subject: add osmo_fsm_inst_update_id_f()
..


add osmo_fsm_inst_update_id_f()

In the osmo-msc, I would like to set the subscr conn FSM identifier by a string
format, to include the type of Complete Layer 3 that is taking place. I could
each time talloc a string and free it again. This API is more convenient.

>From osmo_fsm_inst_update_id(), call osmo_fsm_inst_update_id_f() with "%s" (or
pass NULL).

Put the name updating into separate static update_name() function to clarify.

Adjust the error message for erratic ID: don't say "allocate", it might be from
an update. Adjust test expectation.

Change-Id: I76743a7642f2449fd33350691ac8ebbf4400371d
---
M include/osmocom/core/fsm.h
M src/fsm.c
M tests/fsm/fsm_test.c
M tests/fsm/fsm_test.err
4 files changed, 97 insertions(+), 22 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved



diff --git a/include/osmocom/core/fsm.h b/include/osmocom/core/fsm.h
index 2c2a996..174396a 100644
--- a/include/osmocom/core/fsm.h
+++ b/include/osmocom/core/fsm.h
@@ -159,6 +159,7 @@
 void osmo_fsm_inst_free(struct osmo_fsm_inst *fi);
 
 int osmo_fsm_inst_update_id(struct osmo_fsm_inst *fi, const char *id);
+int osmo_fsm_inst_update_id_f(struct osmo_fsm_inst *fi, const char *fmt, ...);
 
 const char *osmo_fsm_event_name(struct osmo_fsm *fsm, uint32_t event);
 const char *osmo_fsm_inst_name(struct osmo_fsm_inst *fi);
diff --git a/src/fsm.c b/src/fsm.c
index 0fdc564..ae2fdc2 100644
--- a/src/fsm.c
+++ b/src/fsm.c
@@ -211,38 +211,61 @@
  */
 int osmo_fsm_inst_update_id(struct osmo_fsm_inst *fi, const char *id)
 {
-   if (id) {
+   if (!id)
+   return osmo_fsm_inst_update_id_f(fi, NULL);
+   else
+   return osmo_fsm_inst_update_id_f(fi, "%s", id);
+}
+
+static void update_name(struct osmo_fsm_inst *fi)
+{
+   if (fi->name)
+   talloc_free((char*)fi->name);
+
+   if (!fsm_log_addr) {
+   if (fi->id)
+   fi->name = talloc_asprintf(fi, "%s(%s)", fi->fsm->name, 
fi->id);
+   else
+   fi->name = talloc_asprintf(fi, "%s", fi->fsm->name);
+   } else {
+   if (fi->id)
+   fi->name = talloc_asprintf(fi, "%s(%s)[%p]", 
fi->fsm->name, fi->id, fi);
+   else
+   fi->name = talloc_asprintf(fi, "%s[%p]", fi->fsm->name, 
fi);
+   }
+}
+
+/*! Change id of the FSM instance using a string format.
+ * \param[in] fi FSM instance.
+ * \param[in] fmt format string to compose new ID.
+ * \param[in] ... variable argument list for format string.
+ * \returns 0 if the ID was updated, otherwise -EINVAL.
+ */
+int osmo_fsm_inst_update_id_f(struct osmo_fsm_inst *fi, const char *fmt, ...)
+{
+   char *id = NULL;
+
+   if (fmt) {
+   va_list ap;
+
+   va_start(ap, fmt);
+   id = talloc_vasprintf(fi, fmt, ap);
+   va_end(ap);
+
if (!osmo_identifier_valid(id)) {
LOGP(DLGLOBAL, LOGL_ERROR,
 "Attempting to set illegal id for FSM instance of 
type '%s': %s\n",
 fi->fsm->name, osmo_quote_str(id, -1));
+   talloc_free(id);
return -EINVAL;
}
-   osmo_talloc_replace_string(fi, (char **)>id, id);
-
-   if (fi->name)
-   talloc_free((void*)fi->name);
-
-   if (!fsm_log_addr) {
-   fi->name = talloc_asprintf(fi, "%s(%s)", fi->fsm->name, 
id);
-   } else {
-   fi->name = talloc_asprintf(fi, "%s(%s)[%p]", 
fi->fsm->name, id, fi);
-   }
-
-   return 0;
}
 
if (fi->id)
-   talloc_free((void*)fi->id);
-   fi->id = NULL;
-   if (fi->name)
-   talloc_free((void*)fi->name);
+   talloc_free((char*)fi->id);
+   fi->id = id;
 
-   if (!fsm_log_addr) {
-   fi->name = talloc_asprintf(fi, "%s", fi->fsm->name);
-   } else {
-   fi->name = talloc_asprintf(fi, "%s[%p]", fi->fsm->name, fi);
-   }
+   update_name(fi);
return 0;
 }
 
diff --git a/tests/fsm/fsm_test.c b/tests/fsm/fsm_test.c
index 859b78d..e34164c 100644
--- a/tests/fsm/fsm_test.c
+++ b/tests/fsm/fsm_test.c
@@ -237,6 +237,30 @@
 
test_id("invalid.id", -EINVAL, "(arbitrary_id)");
 
+   fprintf(stderr, "--- id format tests...\n");
+/* Update the id, assert the proper rc, and expect a resulting fsm inst name + 
lookup */
+#define test_id_f(expect_rc, expect_name_suffix, new_id_fmt, args...) do { \
+   int rc; \
+   fprintf(stderr, "osmo_fsm_inst_update_id_f(%s, " #args ")\n", \
+   osmo_quote_str(new_id_fmt, -1)); \
+   rc = 

libosmocore[master]: add osmo_fsm_inst_update_id_f()

[Harald Welte]

Patch Set 2: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/7629
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76743a7642f2449fd33350691ac8ebbf4400371d
Gerrit-PatchSet: 2
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

[PATCH] libosmocore[master]: add osmo_fsm_inst_update_id_f()

[Neels Hofmeyr]

Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/7629

to look at the new patch set (#2).

add osmo_fsm_inst_update_id_f()

In the osmo-msc, I would like to set the subscr conn FSM identifier by a string
format, to include the type of Complete Layer 3 that is taking place. I could
each time talloc a string and free it again. This API is more convenient.

>From osmo_fsm_inst_update_id(), call osmo_fsm_inst_update_id_f() with "%s" (or
pass NULL).

Put the name updating into separate static update_name() function to clarify.

Adjust the error message for erratic ID: don't say "allocate", it might be from
an update. Adjust test expectation.

Change-Id: I76743a7642f2449fd33350691ac8ebbf4400371d
---
M include/osmocom/core/fsm.h
M src/fsm.c
M tests/fsm/fsm_test.c
M tests/fsm/fsm_test.err
4 files changed, 97 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/29/7629/2

diff --git a/include/osmocom/core/fsm.h b/include/osmocom/core/fsm.h
index 2c2a996..174396a 100644
--- a/include/osmocom/core/fsm.h
+++ b/include/osmocom/core/fsm.h
@@ -159,6 +159,7 @@
 void osmo_fsm_inst_free(struct osmo_fsm_inst *fi);
 
 int osmo_fsm_inst_update_id(struct osmo_fsm_inst *fi, const char *id);
+int osmo_fsm_inst_update_id_f(struct osmo_fsm_inst *fi, const char *fmt, ...);
 
 const char *osmo_fsm_event_name(struct osmo_fsm *fsm, uint32_t event);
 const char *osmo_fsm_inst_name(struct osmo_fsm_inst *fi);
diff --git a/src/fsm.c b/src/fsm.c
index 0fdc564..ae2fdc2 100644
--- a/src/fsm.c
+++ b/src/fsm.c
@@ -211,38 +211,61 @@
  */
 int osmo_fsm_inst_update_id(struct osmo_fsm_inst *fi, const char *id)
 {
-   if (id) {
+   if (!id)
+   return osmo_fsm_inst_update_id_f(fi, NULL);
+   else
+   return osmo_fsm_inst_update_id_f(fi, "%s", id);
+}
+
+static void update_name(struct osmo_fsm_inst *fi)
+{
+   if (fi->name)
+   talloc_free((char*)fi->name);
+
+   if (!fsm_log_addr) {
+   if (fi->id)
+   fi->name = talloc_asprintf(fi, "%s(%s)", fi->fsm->name, 
fi->id);
+   else
+   fi->name = talloc_asprintf(fi, "%s", fi->fsm->name);
+   } else {
+   if (fi->id)
+   fi->name = talloc_asprintf(fi, "%s(%s)[%p]", 
fi->fsm->name, fi->id, fi);
+   else
+   fi->name = talloc_asprintf(fi, "%s[%p]", fi->fsm->name, 
fi);
+   }
+}
+
+/*! Change id of the FSM instance using a string format.
+ * \param[in] fi FSM instance.
+ * \param[in] fmt format string to compose new ID.
+ * \param[in] ... variable argument list for format string.
+ * \returns 0 if the ID was updated, otherwise -EINVAL.
+ */
+int osmo_fsm_inst_update_id_f(struct osmo_fsm_inst *fi, const char *fmt, ...)
+{
+   char *id = NULL;
+
+   if (fmt) {
+   va_list ap;
+
+   va_start(ap, fmt);
+   id = talloc_vasprintf(fi, fmt, ap);
+   va_end(ap);
+
if (!osmo_identifier_valid(id)) {
LOGP(DLGLOBAL, LOGL_ERROR,
 "Attempting to set illegal id for FSM instance of 
type '%s': %s\n",
 fi->fsm->name, osmo_quote_str(id, -1));
+   talloc_free(id);
return -EINVAL;
}
-   osmo_talloc_replace_string(fi, (char **)>id, id);
-
-   if (fi->name)
-   talloc_free((void*)fi->name);
-
-   if (!fsm_log_addr) {
-   fi->name = talloc_asprintf(fi, "%s(%s)", fi->fsm->name, 
id);
-   } else {
-   fi->name = talloc_asprintf(fi, "%s(%s)[%p]", 
fi->fsm->name, id, fi);
-   }
-
-   return 0;
}
 
if (fi->id)
-   talloc_free((void*)fi->id);
-   fi->id = NULL;
-   if (fi->name)
-   talloc_free((void*)fi->name);
+   talloc_free((char*)fi->id);
+   fi->id = id;
 
-   if (!fsm_log_addr) {
-   fi->name = talloc_asprintf(fi, "%s", fi->fsm->name);
-   } else {
-   fi->name = talloc_asprintf(fi, "%s[%p]", fi->fsm->name, fi);
-   }
+   update_name(fi);
return 0;
 }
 
diff --git a/tests/fsm/fsm_test.c b/tests/fsm/fsm_test.c
index 859b78d..e34164c 100644
--- a/tests/fsm/fsm_test.c
+++ b/tests/fsm/fsm_test.c
@@ -237,6 +237,30 @@
 
test_id("invalid.id", -EINVAL, "(arbitrary_id)");
 
+   fprintf(stderr, "--- id format tests...\n");
+/* Update the id, assert the proper rc, and expect a resulting fsm inst name + 
lookup */
+#define test_id_f(expect_rc, expect_name_suffix, new_id_fmt, args...) do { \
+   int rc; \
+   fprintf(stderr, "osmo_fsm_inst_update_id_f(%s, " #args ")\n", \
+   osmo_quote_str(new_id_fmt, -1)); \
+   rc = 

libosmocore[master]: add osmo_fsm_inst_update_id_f()

[Harald Welte]

Patch Set 1:

Fine with me, but should have unit test coverage, including check for no 
memleak?

-- 
To view, visit https://gerrit.osmocom.org/7629
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76743a7642f2449fd33350691ac8ebbf4400371d
Gerrit-PatchSet: 1
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

libosmocore[master]: add osmo_fsm_inst_update_id_f()

[Neels Hofmeyr]

Patch Set 1:

I'm not sure if this is just code bloat or worth the API real estate. What do 
you guys think?

-- 
To view, visit https://gerrit.osmocom.org/7629
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I76743a7642f2449fd33350691ac8ebbf4400371d
Gerrit-PatchSet: 1
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

[PATCH] libosmocore[master]: add osmo_fsm_inst_update_id_f()

[Neels Hofmeyr]

Review at  https://gerrit.osmocom.org/7629

add osmo_fsm_inst_update_id_f()

In the osmo-msc, I would like to set the subscr conn FSM identifier by a string
format, to include the type of Complete Layer 3 that is taking place. I could
each time talloc a string and free it again. This API is more convenient.

>From osmo_fsm_inst_update_id(), call osmo_fsm_inst_update_id_f() with "%s" (or
pass NULL).

Put the name updating into separate static update_name() function to clarify.

Change-Id: I76743a7642f2449fd33350691ac8ebbf4400371d
---
M include/osmocom/core/fsm.h
M src/fsm.c
2 files changed, 52 insertions(+), 27 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/29/7629/1

diff --git a/include/osmocom/core/fsm.h b/include/osmocom/core/fsm.h
index 2c2a996..174396a 100644
--- a/include/osmocom/core/fsm.h
+++ b/include/osmocom/core/fsm.h
@@ -159,6 +159,7 @@
 void osmo_fsm_inst_free(struct osmo_fsm_inst *fi);
 
 int osmo_fsm_inst_update_id(struct osmo_fsm_inst *fi, const char *id);
+int osmo_fsm_inst_update_id_f(struct osmo_fsm_inst *fi, const char *fmt, ...);
 
 const char *osmo_fsm_event_name(struct osmo_fsm *fsm, uint32_t event);
 const char *osmo_fsm_inst_name(struct osmo_fsm_inst *fi);
diff --git a/src/fsm.c b/src/fsm.c
index 56bb3d0..4b0a3a8 100644
--- a/src/fsm.c
+++ b/src/fsm.c
@@ -206,40 +206,64 @@
  */
 int osmo_fsm_inst_update_id(struct osmo_fsm_inst *fi, const char *id)
 {
-   if (id && !*id)
-   id = NULL;
+   if (!id || !*id)
+   return osmo_fsm_inst_update_id_f(fi, NULL);
+   else
+   return osmo_fsm_inst_update_id_f(fi, "%s", id);
+}
 
-   if (id) {
-   if (!osmo_identifier_valid(id)) {
-   LOGP(DLGLOBAL, LOGL_ERROR, "Attempting to allocate FSM 
instance of type '%s'"
-" with illegal identifier '%s'\n", fi->fsm->name, 
id);
+static void update_name(struct osmo_fsm_inst *fi)
+{
+   if (fi->name)
+   talloc_free((char*)fi->name);
+
+   if (!fsm_log_addr) {
+   if (fi->id)
+   fi->name = talloc_asprintf(fi, "%s(%s)", fi->fsm->name, 
fi->id);
+   else
+   fi->name = talloc_asprintf(fi, "%s", fi->fsm->name);
+   } else {
+   if (fi->id)
+   fi->name = talloc_asprintf(fi, "%s(%s)[%p]", 
fi->fsm->name, fi->id, fi);
+   else
+   fi->name = talloc_asprintf(fi, "%s[%p]", fi->fsm->name, 
fi);
+   }
+}
+
+/*! Change id of the FSM instance using a string format.
+ * \param[in] fi FSM instance.
+ * \param[in] fmt format string to compose new ID.
+ * \param[in] ... variable argument list for format string.
+ * \returns 0 if the ID was updated, otherwise -EINVAL.
+ */
+int osmo_fsm_inst_update_id_f(struct osmo_fsm_inst *fi, const char *fmt, ...)
+{
+   char *id = NULL;
+
+   if (fmt) {
+   va_list ap;
+
+   va_start(ap, fmt);
+   id = talloc_vasprintf(fi, fmt, ap);
+   va_end(ap);
+
+   if (!*id) {
+   talloc_free(id);
+   id = NULL;
+   } else if (!osmo_identifier_valid(id)) {
+   LOGP(DLGLOBAL, LOGL_ERROR,
+"Attempting to set illegal id for FSM instance of 
type '%s': '%s'\n",
+fi->fsm->name, id);
+   talloc_free(id);
return -EINVAL;
}
-   osmo_talloc_replace_string(fi, (char **)>id, id);
-
-   if (fi->name)
-   talloc_free((void*)fi->name);
-
-   if (!fsm_log_addr) {
-   fi->name = talloc_asprintf(fi, "%s(%s)", fi->fsm->name, 
id);
-   } else {
-   fi->name = talloc_asprintf(fi, "%s(%s)[%p]", 
fi->fsm->name, id, fi);
-   }
-
-   return 0;
}
 
if (fi->id)
-   talloc_free((void*)fi->id);
-   fi->id = NULL;
-   if (fi->name)
-   talloc_free((void*)fi->name);
+   talloc_free((char*)fi->id);
+   fi->id = id;
 
-   if (!fsm_log_addr) {
-   fi->name = talloc_asprintf(fi, "%s", fi->fsm->name);
-   } else {
-   fi->name = talloc_asprintf(fi, "%s[%p]", fi->fsm->name, fi);
-   }
+   update_name(fi);
return 0;
 }
 

-- 
To view, visit https://gerrit.osmocom.org/7629
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I76743a7642f2449fd33350691ac8ebbf4400371d
Gerrit-PatchSet: 1
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr