Skip to content

Commit 10dc82b

Browse files
nrwahl2nrwahl2
committed
Refactor: libcrmcommon: Use g_strsplit() in crm_parse_agent_spec()
This seems clearer than fiddling with strchr(). Signed-off-by: Reid Wahl <[email protected]>
1 parent 8d5ea8b commit 10dc82b

File tree

2 files changed

+87
-49
lines changed

2 files changed

+87
-49
lines changed

‎lib/common/agents.c‎

Lines changed: 49 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -105,53 +105,76 @@ crm_generate_ra_key(const char *standard, const char *provider,
105105
* \brief Parse a "standard[:provider]:type" agent specification
106106
*
107107
* \param[in] spec Agent specification
108-
* \param[out] standard Newly allocated memory containing agent standard (or NULL)
109-
* \param[out] provider Newly allocated memory containing agent provider (or NULL)
110-
* \param[put] type Newly allocated memory containing agent type (or NULL)
108+
* \param[out] standard Where to store agent standard (may not be \c NULL)
109+
* \param[out] provider Where to store agent provider if the standard supports
110+
* one (may not be \c NULL)
111+
* \param[put] type Where to store agent type (may not be \c NULL)
111112
*
112-
* \return pcmk_ok if the string could be parsed, -EINVAL otherwise
113+
* \return \c pcmk_ok if the string could be parsed, \c -EINVAL otherwise
113114
*
114115
* \note It is acceptable for the type to contain a ':' if the standard supports
115116
* that. For example, systemd supports the form "systemd:UNIT@A:B".
116-
* \note It is the caller's responsibility to free the returned values.
117+
* \note On success, the caller is responsible for freeing \p *standard,
118+
* \p *provider, and \p *type using \c free(). On failure, all of these
119+
* are set to \c NULL.
117120
*/
118121
int
119122
crm_parse_agent_spec(const char *spec, char **standard, char **provider,
120123
char **type)
121124
{
122-
const char *colon = NULL;
125+
gchar **parts = NULL;
126+
int rc = pcmk_ok;
123127

124-
CRM_CHECK(spec && standard && provider && type, return -EINVAL);
128+
CRM_CHECK((spec != NULL) && (standard != NULL) && (provider != NULL)
129+
&& (type != NULL), return -EINVAL);
125130
*standard = NULL;
126131
*provider = NULL;
127132
*type = NULL;
128133

129-
colon = strchr(spec, ':');
130-
if ((colon == NULL) || (colon == spec)) {
131-
return -EINVAL;
134+
parts = g_strsplit(spec, ":", 3);
135+
136+
if (pcmk__str_empty(parts[0])) {
137+
// Empty standard
138+
rc = -EINVAL;
139+
goto done;
132140
}
133141

134-
*standard = strndup(spec, colon - spec);
135-
spec = colon + 1;
142+
if (pcmk__is_set(pcmk_get_ra_caps(parts[0]), pcmk_ra_cap_provider)) {
143+
if (pcmk__str_empty(parts[1]) || pcmk__str_empty(parts[2])) {
144+
// Empty provider or type
145+
rc = -EINVAL;
146+
goto done;
147+
}
148+
149+
*standard = pcmk__str_copy(parts[0]);
150+
*provider = pcmk__str_copy(parts[1]);
151+
*type = pcmk__str_copy(parts[2]);
136152

137-
if (pcmk__is_set(pcmk_get_ra_caps(*standard), pcmk_ra_cap_provider)) {
138-
colon = strchr(spec, ':');
139-
if ((colon == NULL) || (colon == spec)) {
140-
free(*standard);
141-
return -EINVAL;
153+
} else {
154+
if (pcmk__str_empty(parts[1])) {
155+
// Empty type
156+
rc = -EINVAL;
157+
goto done;
142158
}
143-
*provider = strndup(spec, colon - spec);
144-
spec = colon + 1;
145-
}
146159

147-
if (*spec == '\0') {
148-
free(*standard);
149-
free(*provider);
150-
return -EINVAL;
160+
*standard = pcmk__str_copy(parts[0]);
161+
162+
if (parts[2] == NULL) {
163+
// Common case: type does not contain a colon
164+
*type = pcmk__str_copy(parts[1]);
165+
166+
} else {
167+
// Accommodate "systemd:UNIT@A:B", for example
168+
gchar *joined = g_strjoinv(":", parts + 1);
169+
170+
*type = pcmk__str_copy(joined);
171+
g_free(joined);
172+
}
151173
}
152174

153-
*type = strdup(spec);
154-
return pcmk_ok;
175+
done:
176+
g_strfreev(parts);
177+
return rc;
155178
}
156179

157180
/*!

‎lib/common/tests/agents/crm_parse_agent_spec_test.c‎

Lines changed: 38 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,21 @@
1212
#include <crm/common/unittest_internal.h>
1313
#include <crm/common/agents.h>
1414

15+
/*!
16+
* \internal
17+
* \brief Parse an agent spec and compare against expected values
18+
*
19+
* \param[in] line Line number of caller (for error output)
20+
* \param[in] spec Agent spec
21+
* \param[in] expected_std Expected standard
22+
* \param[in] expected_prov Expected provider
23+
* \param[in] expected_type Expected type
24+
* \param[in] expected_rc Expected return code of \c crm_parse_agent_spec()
25+
*/
1526
static void
1627
assert_parse_agent_spec_as(int line, const char *spec, const char *expected_std,
1728
const char *expected_prov, const char *expected_type,
18-
int expected_rc, bool check_spt)
29+
int expected_rc)
1930
{
2031
char *std = NULL;
2132
char *prov = NULL;
@@ -43,40 +54,46 @@ assert_parse_agent_spec_as(int line, const char *spec, const char *expected_std,
4354
cast_ptr_to_largest_integral_type(NULL), \
4455
__FILE__, (line))
4556

46-
if (!check_spt) {
47-
/* This is a temporary hack to work around an issue that will be fixed
48-
* in an upcoming commit
49-
*/
50-
return;
51-
}
52-
5357
if (expected_std == NULL) {
5458
pcmk__assert_null(std);
59+
5560
} else {
5661
_assert_string_equal(std, expected_std, __FILE__, line);
5762
free(std);
5863
}
5964

6065
if (expected_prov == NULL) {
6166
pcmk__assert_null(prov);
67+
6268
} else {
6369
_assert_string_equal(prov, expected_prov, __FILE__, line);
6470
free(prov);
6571
}
6672

6773
if (expected_type == NULL) {
6874
pcmk__assert_null(type);
75+
6976
} else {
7077
_assert_string_equal(type, expected_type, __FILE__, line);
7178
free(type);
7279
}
7380
}
7481

75-
#define assert_parse_agent_spec(spec, expected_std, expected_prov, \
76-
expected_type, expected_rc, check_spt) \
77-
assert_parse_agent_spec_as(__LINE__, (spec), (expected_std), \
78-
(expected_prov), (expected_type), \
79-
(expected_rc), (check_spt))
82+
/*!
83+
* \internal
84+
* \brief Parse an agent spec and compare against expected values
85+
*
86+
* \param[in] spec Agent spec
87+
* \param[in] expected_std Expected standard
88+
* \param[in] expected_prov Expected provider
89+
* \param[in] expected_type Expected type
90+
* \param[in] expected_rc Expected return code of \c crm_parse_agent_spec()
91+
*/
92+
#define assert_parse_agent_spec(spec, expected_std, expected_prov, \
93+
expected_type, expected_rc) \
94+
assert_parse_agent_spec_as(__LINE__, (spec), (expected_std), \
95+
(expected_prov), (expected_type), \
96+
(expected_rc))
8097

8198
static void
8299
null_params(void **state)
@@ -139,11 +156,9 @@ null_params(void **state)
139156
static void
140157
no_prov_or_type(void **state)
141158
{
142-
assert_parse_agent_spec("ocf", NULL, NULL, NULL, -EINVAL, true);
143-
144-
// @FIXME std is freed on error, so set it to NULL or don't check its value
145-
assert_parse_agent_spec("ocf:", NULL, NULL, NULL, -EINVAL, false);
146-
assert_parse_agent_spec("ocf::", NULL, NULL, NULL, -EINVAL, false);
159+
assert_parse_agent_spec("ocf", NULL, NULL, NULL, -EINVAL);
160+
assert_parse_agent_spec("ocf:", NULL, NULL, NULL, -EINVAL);
161+
assert_parse_agent_spec("ocf::", NULL, NULL, NULL, -EINVAL);
147162
}
148163

149164
static void
@@ -152,28 +167,28 @@ no_type(void **state)
152167
/* @FIXME std and prov are freed on error, so set them to NULL or don't
153168
* check their values
154169
*/
155-
assert_parse_agent_spec("ocf:pacemaker:", NULL, NULL, NULL, -EINVAL, false);
170+
assert_parse_agent_spec("ocf:pacemaker:", NULL, NULL, NULL, -EINVAL);
156171
}
157172

158173
static void
159174
get_std_and_ty(void **state)
160175
{
161176
assert_parse_agent_spec("stonith:fence_xvm", "stonith", NULL, "fence_xvm",
162-
pcmk_ok, true);
177+
pcmk_ok);
163178
}
164179

165180
static void
166181
get_all_values(void **state)
167182
{
168183
assert_parse_agent_spec("ocf:pacemaker:ping", "ocf", "pacemaker", "ping",
169-
pcmk_ok, true);
184+
pcmk_ok);
170185
}
171186

172187
static void
173188
get_systemd_values(void **state)
174189
{
175190
assert_parse_agent_spec("systemd:UNIT@A:B", "systemd", NULL, "UNIT@A:B",
176-
pcmk_ok, true);
191+
pcmk_ok);
177192
}
178193

179194
static void
@@ -184,7 +199,7 @@ type_ends_with_colon(void **state)
184199
* should be considered the type. This includes a trailing colon.
185200
*/
186201
assert_parse_agent_spec("stonith:fence_xvm:", "stonith", NULL, "fence_xvm:",
187-
pcmk_ok, true);
202+
pcmk_ok);
188203
}
189204

190205
PCMK__UNIT_TEST(NULL, NULL,

0 commit comments

Comments
 (0)