summaryrefslogtreecommitdiff
path: root/ovn
diff options
context:
space:
mode:
authorNuman Siddique <nusiddiq@redhat.com>2017-11-02 06:18:34 +0530
committerBen Pfaff <blp@ovn.org>2017-11-02 11:42:33 -0700
commit16936e4d6e3f8dd68dd6a4959e4937c9daab3e4a (patch)
treed442f4ba80f77a01d3359e36a674a18b673385d7 /ovn
parent214117fdbd1f2d2766757d5b29e923cfcaadc914 (diff)
downloadopenvswitch-16936e4d6e3f8dd68dd6a4959e4937c9daab3e4a.tar.gz
ovn util: Refactor dhcp_opts_map to make it generic
Renamed 'struct dhcp_opts_map' to 'struct gen_opts_map' and renamed ovn-dhcp.h to ovn-l7.h. An upcoming commit to support IPv6 Router Advertisement, will make use of the refactored code to store the IPv6 ND RA options in 'struct gen_opts_map'. Signed-off-by: Numan Siddique <nusiddiq@redhat.com> Acked-by: Miguel Angel Ajo <majopela@redhat.com> Signed-off-by: Ben Pfaff <blp@ovn.org>
Diffstat (limited to 'ovn')
-rw-r--r--ovn/controller/lflow.c2
-rw-r--r--ovn/controller/pinctrl.c2
-rw-r--r--ovn/lib/actions.c100
-rw-r--r--ovn/lib/automake.mk2
-rw-r--r--ovn/lib/ovn-l7.h (renamed from ovn/lib/ovn-dhcp.h)67
-rw-r--r--ovn/northd/ovn-northd.c14
-rw-r--r--ovn/utilities/ovn-trace.c14
7 files changed, 119 insertions, 82 deletions
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 20a18c259..6b6b91abc 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -25,7 +25,7 @@
#include "ovn-controller.h"
#include "ovn/actions.h"
#include "ovn/expr.h"
-#include "ovn/lib/ovn-dhcp.h"
+#include "ovn/lib/ovn-l7.h"
#include "ovn/lib/ovn-sb-idl.h"
#include "packets.h"
#include "physical.h"
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index f9b2e0caf..6218d8a92 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -41,7 +41,7 @@
#include "ovn/lex.h"
#include "ovn/lib/acl-log.h"
#include "ovn/lib/logical-fields.h"
-#include "ovn/lib/ovn-dhcp.h"
+#include "ovn/lib/ovn-l7.h"
#include "ovn/lib/ovn-util.h"
#include "poll-loop.h"
#include "openvswitch/rconn.h"
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 2aace1487..d4cde6acb 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -20,7 +20,7 @@
#include "bitmap.h"
#include "byte-order.h"
#include "compiler.h"
-#include "ovn-dhcp.h"
+#include "ovn-l7.h"
#include "hash.h"
#include "logical-fields.h"
#include "nx-match.h"
@@ -1432,19 +1432,17 @@ ovnact_put_mac_bind_free(struct ovnact_put_mac_bind *put_mac OVS_UNUSED)
}
static void
-parse_dhcp_opt(struct action_context *ctx, struct ovnact_dhcp_option *o,
- bool v6)
+parse_gen_opt(struct action_context *ctx, struct ovnact_gen_option *o,
+ const struct hmap *gen_opts, const char *opts_type)
{
if (ctx->lexer->token.type != LEX_T_ID) {
lexer_syntax_error(ctx->lexer, NULL);
return;
}
- const char *name = v6 ? "DHCPv6" : "DHCPv4";
- const struct hmap *map = v6 ? ctx->pp->dhcpv6_opts : ctx->pp->dhcp_opts;
- o->option = map ? dhcp_opts_find(map, ctx->lexer->token.s) : NULL;
+ o->option = gen_opts ? gen_opts_find(gen_opts, ctx->lexer->token.s) : NULL;
if (!o->option) {
- lexer_syntax_error(ctx->lexer, "expecting %s option name", name);
+ lexer_syntax_error(ctx->lexer, "expecting %s option name", opts_type);
return;
}
lexer_get(ctx->lexer);
@@ -1461,22 +1459,22 @@ parse_dhcp_opt(struct action_context *ctx, struct ovnact_dhcp_option *o,
if (!strcmp(o->option->type, "str")) {
if (o->value.type != EXPR_C_STRING) {
lexer_error(ctx->lexer, "%s option %s requires string value.",
- name, o->option->name);
+ opts_type, o->option->name);
return;
}
} else {
if (o->value.type != EXPR_C_INTEGER) {
lexer_error(ctx->lexer, "%s option %s requires numeric value.",
- name, o->option->name);
+ opts_type, o->option->name);
return;
}
}
}
-static const struct ovnact_dhcp_option *
-find_offerip(const struct ovnact_dhcp_option *options, size_t n)
+static const struct ovnact_gen_option *
+find_offerip(const struct ovnact_gen_option *options, size_t n)
{
- for (const struct ovnact_dhcp_option *o = options; o < &options[n]; o++) {
+ for (const struct ovnact_gen_option *o = options; o < &options[n]; o++) {
if (o->option->code == 0) {
return o;
}
@@ -1485,21 +1483,18 @@ find_offerip(const struct ovnact_dhcp_option *options, size_t n)
}
static void
-free_dhcp_options(struct ovnact_dhcp_option *options, size_t n)
+free_gen_options(struct ovnact_gen_option *options, size_t n)
{
- for (struct ovnact_dhcp_option *o = options; o < &options[n]; o++) {
+ for (struct ovnact_gen_option *o = options; o < &options[n]; o++) {
expr_constant_set_destroy(&o->value);
}
free(options);
}
-/* Parses the "put_dhcp_opts" and "put_dhcpv6_opts" actions.
- *
- * The caller has already consumed "<dst> =", so this just parses the rest. */
static void
-parse_put_dhcp_opts(struct action_context *ctx,
- const struct expr_field *dst,
- struct ovnact_put_dhcp_opts *pdo)
+parse_put_opts(struct action_context *ctx, const struct expr_field *dst,
+ struct ovnact_put_opts *po, const struct hmap *gen_opts,
+ const char *opts_type)
{
lexer_get(ctx->lexer); /* Skip put_dhcp[v6]_opts. */
lexer_get(ctx->lexer); /* Skip '('. */
@@ -1511,27 +1506,44 @@ parse_put_dhcp_opts(struct action_context *ctx,
free(error);
return;
}
- pdo->dst = *dst;
+ po->dst = *dst;
size_t allocated_options = 0;
while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
- if (pdo->n_options >= allocated_options) {
- pdo->options = x2nrealloc(pdo->options, &allocated_options,
- sizeof *pdo->options);
+ if (po->n_options >= allocated_options) {
+ po->options = x2nrealloc(po->options, &allocated_options,
+ sizeof *po->options);
}
- struct ovnact_dhcp_option *o = &pdo->options[pdo->n_options++];
+ struct ovnact_gen_option *o = &po->options[po->n_options++];
memset(o, 0, sizeof *o);
- parse_dhcp_opt(ctx, o, pdo->ovnact.type == OVNACT_PUT_DHCPV6_OPTS);
+ parse_gen_opt(ctx, o, gen_opts, opts_type);
if (ctx->lexer->error) {
return;
}
lexer_match(ctx->lexer, LEX_T_COMMA);
}
+}
+
+/* Parses the "put_dhcp_opts" and "put_dhcpv6_opts" actions.
+ *
+ * The caller has already consumed "<dst> =", so this just parses the rest. */
+static void
+parse_put_dhcp_opts(struct action_context *ctx,
+ const struct expr_field *dst,
+ struct ovnact_put_opts *po)
+{
+ const struct hmap *dhcp_opts =
+ (po->ovnact.type == OVNACT_PUT_DHCPV6_OPTS) ?
+ ctx->pp->dhcpv6_opts : ctx->pp->dhcp_opts;
+ const char *opts_type =
+ (po->ovnact.type == OVNACT_PUT_DHCPV6_OPTS) ? "DHCPv6" : "DHCPv4";
+
+ parse_put_opts(ctx, dst, po, dhcp_opts, opts_type);
- if (pdo->ovnact.type == OVNACT_PUT_DHCPV4_OPTS
- && !find_offerip(pdo->options, pdo->n_options)) {
+ if (!ctx->lexer->error && po->ovnact.type == OVNACT_PUT_DHCPV4_OPTS
+ && !find_offerip(po->options, po->n_options)) {
lexer_error(ctx->lexer,
"put_dhcp_opts requires offerip to be specified.");
return;
@@ -1539,12 +1551,12 @@ parse_put_dhcp_opts(struct action_context *ctx,
}
static void
-format_put_dhcp_opts(const char *name,
- const struct ovnact_put_dhcp_opts *pdo, struct ds *s)
+format_put_opts(const char *name, const struct ovnact_put_opts *pdo,
+ struct ds *s)
{
expr_field_format(&pdo->dst, s);
ds_put_format(s, " = %s(", name);
- for (const struct ovnact_dhcp_option *o = pdo->options;
+ for (const struct ovnact_gen_option *o = pdo->options;
o < &pdo->options[pdo->n_options]; o++) {
if (o != pdo->options) {
ds_put_cstr(s, ", ");
@@ -1556,19 +1568,19 @@ format_put_dhcp_opts(const char *name,
}
static void
-format_PUT_DHCPV4_OPTS(const struct ovnact_put_dhcp_opts *pdo, struct ds *s)
+format_PUT_DHCPV4_OPTS(const struct ovnact_put_opts *pdo, struct ds *s)
{
- format_put_dhcp_opts("put_dhcp_opts", pdo, s);
+ format_put_opts("put_dhcp_opts", pdo, s);
}
static void
-format_PUT_DHCPV6_OPTS(const struct ovnact_put_dhcp_opts *pdo, struct ds *s)
+format_PUT_DHCPV6_OPTS(const struct ovnact_put_opts *pdo, struct ds *s)
{
- format_put_dhcp_opts("put_dhcpv6_opts", pdo, s);
+ format_put_opts("put_dhcpv6_opts", pdo, s);
}
static void
-encode_put_dhcpv4_option(const struct ovnact_dhcp_option *o,
+encode_put_dhcpv4_option(const struct ovnact_gen_option *o,
struct ofpbuf *ofpacts)
{
uint8_t *opt_header = ofpbuf_put_zeros(ofpacts, 2);
@@ -1644,7 +1656,7 @@ encode_put_dhcpv4_option(const struct ovnact_dhcp_option *o,
}
static void
-encode_put_dhcpv6_option(const struct ovnact_dhcp_option *o,
+encode_put_dhcpv6_option(const struct ovnact_gen_option *o,
struct ofpbuf *ofpacts)
{
struct dhcp_opt6_header *opt = ofpbuf_put_uninit(ofpacts, sizeof *opt);
@@ -1672,7 +1684,7 @@ encode_put_dhcpv6_option(const struct ovnact_dhcp_option *o,
}
static void
-encode_PUT_DHCPV4_OPTS(const struct ovnact_put_dhcp_opts *pdo,
+encode_PUT_DHCPV4_OPTS(const struct ovnact_put_opts *pdo,
const struct ovnact_encode_params *ep OVS_UNUSED,
struct ofpbuf *ofpacts)
{
@@ -1687,12 +1699,12 @@ encode_PUT_DHCPV4_OPTS(const struct ovnact_put_dhcp_opts *pdo,
/* Encode the offerip option first, because it's a special case and needs
* to be first in the actual DHCP response, and then encode the rest
* (skipping offerip the second time around). */
- const struct ovnact_dhcp_option *offerip_opt = find_offerip(
+ const struct ovnact_gen_option *offerip_opt = find_offerip(
pdo->options, pdo->n_options);
ovs_be32 offerip = offerip_opt->value.values[0].value.ipv4;
ofpbuf_put(ofpacts, &offerip, sizeof offerip);
- for (const struct ovnact_dhcp_option *o = pdo->options;
+ for (const struct ovnact_gen_option *o = pdo->options;
o < &pdo->options[pdo->n_options]; o++) {
if (o != offerip_opt) {
encode_put_dhcpv4_option(o, ofpacts);
@@ -1703,7 +1715,7 @@ encode_PUT_DHCPV4_OPTS(const struct ovnact_put_dhcp_opts *pdo,
}
static void
-encode_PUT_DHCPV6_OPTS(const struct ovnact_put_dhcp_opts *pdo,
+encode_PUT_DHCPV6_OPTS(const struct ovnact_put_opts *pdo,
const struct ovnact_encode_params *ep OVS_UNUSED,
struct ofpbuf *ofpacts)
{
@@ -1715,7 +1727,7 @@ encode_PUT_DHCPV6_OPTS(const struct ovnact_put_dhcp_opts *pdo,
ovs_be32 ofs = htonl(dst.ofs);
ofpbuf_put(ofpacts, &ofs, sizeof ofs);
- for (const struct ovnact_dhcp_option *o = pdo->options;
+ for (const struct ovnact_gen_option *o = pdo->options;
o < &pdo->options[pdo->n_options]; o++) {
encode_put_dhcpv6_option(o, ofpacts);
}
@@ -1724,9 +1736,9 @@ encode_PUT_DHCPV6_OPTS(const struct ovnact_put_dhcp_opts *pdo,
}
static void
-ovnact_put_dhcp_opts_free(struct ovnact_put_dhcp_opts *pdo)
+ovnact_put_opts_free(struct ovnact_put_opts *pdo)
{
- free_dhcp_options(pdo->options, pdo->n_options);
+ free_gen_options(pdo->options, pdo->n_options);
}
static void
diff --git a/ovn/lib/automake.mk b/ovn/lib/automake.mk
index d9ed050f3..dae06a5c1 100644
--- a/ovn/lib/automake.mk
+++ b/ovn/lib/automake.mk
@@ -11,7 +11,7 @@ ovn_lib_libovn_la_SOURCES = \
ovn/lib/chassis-index.h \
ovn/lib/expr.c \
ovn/lib/lex.c \
- ovn/lib/ovn-dhcp.h \
+ ovn/lib/ovn-l7.h \
ovn/lib/ovn-util.c \
ovn/lib/ovn-util.h \
ovn/lib/logical-fields.c \
diff --git a/ovn/lib/ovn-dhcp.h b/ovn/lib/ovn-l7.h
index d5561edfa..40bd75461 100644
--- a/ovn/lib/ovn-dhcp.h
+++ b/ovn/lib/ovn-l7.h
@@ -21,7 +21,8 @@
#include "openvswitch/hmap.h"
#include "hash.h"
-struct dhcp_opts_map {
+/* Generic options map which is used to store dhcpv4 opts and dhcpv6 opts. */
+struct gen_opts_map {
struct hmap_node hmap_node;
char *name;
char *type;
@@ -68,45 +69,69 @@ struct dhcp_opts_map {
#define DHCP_OPT_T2 DHCP_OPTION("T2", 59, "uint32")
static inline uint32_t
-dhcp_opt_hash(char *opt_name)
+gen_opt_hash(char *opt_name)
{
return hash_string(opt_name, 0);
}
-static inline struct dhcp_opts_map *
-dhcp_opts_find(const struct hmap *dhcp_opts, char *opt_name)
+static inline uint32_t
+dhcp_opt_hash(char *opt_name)
{
- struct dhcp_opts_map *dhcp_opt;
- HMAP_FOR_EACH_WITH_HASH (dhcp_opt, hmap_node, dhcp_opt_hash(opt_name),
- dhcp_opts) {
- if (!strcmp(dhcp_opt->name, opt_name)) {
- return dhcp_opt;
+ return gen_opt_hash(opt_name);
+}
+
+static inline struct gen_opts_map *
+gen_opts_find(const struct hmap *gen_opts, char *opt_name)
+{
+ struct gen_opts_map *gen_opt;
+ HMAP_FOR_EACH_WITH_HASH (gen_opt, hmap_node, gen_opt_hash(opt_name),
+ gen_opts) {
+ if (!strcmp(gen_opt->name, opt_name)) {
+ return gen_opt;
}
}
return NULL;
}
+static inline struct gen_opts_map *
+dhcp_opts_find(const struct hmap *dhcp_opts, char *opt_name)
+{
+ return gen_opts_find(dhcp_opts, opt_name);
+}
+
+static inline void
+gen_opt_add(struct hmap *gen_opts, char *opt_name, size_t code, char *type)
+{
+ struct gen_opts_map *gen_opt = xzalloc(sizeof *gen_opt);
+ gen_opt->name = xstrdup(opt_name);
+ gen_opt->code = code;
+ gen_opt->type = xstrdup(type);
+ hmap_insert(gen_opts, &gen_opt->hmap_node, gen_opt_hash(opt_name));
+}
+
static inline void
dhcp_opt_add(struct hmap *dhcp_opts, char *opt_name, size_t code, char *type)
{
- struct dhcp_opts_map *dhcp_opt = xzalloc(sizeof *dhcp_opt);
- dhcp_opt->name = xstrdup(opt_name);
- dhcp_opt->code = code;
- dhcp_opt->type = xstrdup(type);
- hmap_insert(dhcp_opts, &dhcp_opt->hmap_node, dhcp_opt_hash(opt_name));
+ gen_opt_add(dhcp_opts, opt_name, code, type);
}
static inline void
-dhcp_opts_destroy(struct hmap *dhcp_opts)
+gen_opts_destroy(struct hmap *gen_opts)
{
- struct dhcp_opts_map *dhcp_opt;
- HMAP_FOR_EACH_POP(dhcp_opt, hmap_node, dhcp_opts) {
- free(dhcp_opt->name);
- free(dhcp_opt->type);
- free(dhcp_opt);
+ struct gen_opts_map *gen_opt;
+ HMAP_FOR_EACH_POP (gen_opt, hmap_node, gen_opts) {
+ free(gen_opt->name);
+ free(gen_opt->type);
+ free(gen_opt);
}
- hmap_destroy(dhcp_opts);
+ hmap_destroy(gen_opts);
+}
+
+static inline void
+dhcp_opts_destroy(struct hmap *dhcp_opts)
+{
+ gen_opts_destroy(dhcp_opts);
}
/* Used in the OpenFlow PACKET_IN userdata */
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 6732ad003..0e00cc061 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -30,7 +30,7 @@
#include "ovn/lex.h"
#include "ovn/lib/chassis-index.h"
#include "ovn/lib/logical-fields.h"
-#include "ovn/lib/ovn-dhcp.h"
+#include "ovn/lib/ovn-l7.h"
#include "ovn/lib/ovn-nb-idl.h"
#include "ovn/lib/ovn-sb-idl.h"
#include "ovn/lib/ovn-util.h"
@@ -6111,7 +6111,7 @@ update_logical_port_status(struct northd_context *ctx)
hmap_destroy(&lports_hmap);
}
-static struct dhcp_opts_map supported_dhcp_opts[] = {
+static struct gen_opts_map supported_dhcp_opts[] = {
OFFERIP,
DHCP_OPT_NETMASK,
DHCP_OPT_ROUTER,
@@ -6138,7 +6138,7 @@ static struct dhcp_opts_map supported_dhcp_opts[] = {
DHCP_OPT_T2
};
-static struct dhcp_opts_map supported_dhcpv6_opts[] = {
+static struct gen_opts_map supported_dhcpv6_opts[] = {
DHCPV6_OPT_IA_ADDR,
DHCPV6_OPT_SERVER_ID,
DHCPV6_OPT_DOMAIN_SEARCH,
@@ -6157,7 +6157,7 @@ check_and_add_supported_dhcp_opts_to_sb_db(struct northd_context *ctx)
const struct sbrec_dhcp_options *opt_row, *opt_row_next;
SBREC_DHCP_OPTIONS_FOR_EACH_SAFE(opt_row, opt_row_next, ctx->ovnsb_idl) {
- struct dhcp_opts_map *dhcp_opt =
+ struct gen_opts_map *dhcp_opt =
dhcp_opts_find(&dhcp_opts_to_add, opt_row->name);
if (dhcp_opt) {
hmap_remove(&dhcp_opts_to_add, &dhcp_opt->hmap_node);
@@ -6166,7 +6166,7 @@ check_and_add_supported_dhcp_opts_to_sb_db(struct northd_context *ctx)
}
}
- struct dhcp_opts_map *opt;
+ struct gen_opts_map *opt;
HMAP_FOR_EACH (opt, hmap_node, &dhcp_opts_to_add) {
struct sbrec_dhcp_options *sbrec_dhcp_option =
sbrec_dhcp_options_insert(ctx->ovnsb_txn);
@@ -6190,7 +6190,7 @@ check_and_add_supported_dhcpv6_opts_to_sb_db(struct northd_context *ctx)
const struct sbrec_dhcpv6_options *opt_row, *opt_row_next;
SBREC_DHCPV6_OPTIONS_FOR_EACH_SAFE(opt_row, opt_row_next, ctx->ovnsb_idl) {
- struct dhcp_opts_map *dhcp_opt =
+ struct gen_opts_map *dhcp_opt =
dhcp_opts_find(&dhcpv6_opts_to_add, opt_row->name);
if (dhcp_opt) {
hmap_remove(&dhcpv6_opts_to_add, &dhcp_opt->hmap_node);
@@ -6199,7 +6199,7 @@ check_and_add_supported_dhcpv6_opts_to_sb_db(struct northd_context *ctx)
}
}
- struct dhcp_opts_map *opt;
+ struct gen_opts_map *opt;
HMAP_FOR_EACH(opt, hmap_node, &dhcpv6_opts_to_add) {
struct sbrec_dhcpv6_options *sbrec_dhcpv6_option =
sbrec_dhcpv6_options_insert(ctx->ovnsb_txn);
diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c
index 59083eebe..d9465c90c 100644
--- a/ovn/utilities/ovn-trace.c
+++ b/ovn/utilities/ovn-trace.c
@@ -36,8 +36,8 @@
#include "ovn/lex.h"
#include "ovn/lib/acl-log.h"
#include "ovn/lib/logical-fields.h"
+#include "ovn/lib/ovn-l7.h"
#include "ovn/lib/ovn-sb-idl.h"
-#include "ovn/lib/ovn-dhcp.h"
#include "ovn/lib/ovn-util.h"
#include "ovsdb-idl.h"
#include "poll-loop.h"
@@ -418,8 +418,8 @@ static struct shash symtab;
static struct shash address_sets;
/* DHCP options. */
-static struct hmap dhcp_opts; /* Contains "struct dhcp_opts_map"s. */
-static struct hmap dhcpv6_opts; /* Contains "struct dhcp_opts_map"s. */
+static struct hmap dhcp_opts; /* Contains "struct gen_opts_map"s. */
+static struct hmap dhcpv6_opts; /* Contains "struct gen_opts_map"s. */
static struct ovntrace_datapath *
ovntrace_datapath_find_by_sb_uuid(const struct uuid *sb_uuid)
@@ -867,7 +867,7 @@ read_flows(void)
}
static void
-read_dhcp_opts(void)
+read_gen_opts(void)
{
hmap_init(&dhcp_opts);
const struct sbrec_dhcp_options *sdo;
@@ -931,7 +931,7 @@ read_db(void)
read_ports();
read_mcgroups();
read_address_sets();
- read_dhcp_opts();
+ read_gen_opts();
read_flows();
read_mac_bindings();
}
@@ -1541,7 +1541,7 @@ execute_get_mac_bind(const struct ovnact_get_mac_bind *bind,
}
static void
-execute_put_dhcp_opts(const struct ovnact_put_dhcp_opts *pdo,
+execute_put_dhcp_opts(const struct ovnact_put_opts *pdo,
const char *name, struct flow *uflow,
struct ovs_list *super)
{
@@ -1551,7 +1551,7 @@ execute_put_dhcp_opts(const struct ovnact_put_dhcp_opts *pdo,
/* Format the put_dhcp_opts action. */
struct ds s = DS_EMPTY_INITIALIZER;
- for (const struct ovnact_dhcp_option *o = pdo->options;
+ for (const struct ovnact_gen_option *o = pdo->options;
o < &pdo->options[pdo->n_options]; o++) {
if (o != pdo->options) {
ds_put_cstr(&s, ", ");