> -----Original Message-----
> From: Aaron Conole [mailto:acon...@redhat.com]
> Sent: Friday, December 4, 2015 3:37 PM
> To: Traynor, Kevin
> Cc: dev@openvswitch.org; Flavio Leitner
> Subject: Re: [ovs-dev] [RFC 1/2] dpdk: Convert initialization from cmdline to
> db
>
> "Traynor, Kevin" <kevin.tray...@intel.com> writes:
> >> -----Original Message-----
> <<snipped>>
> >> +static int
> >> +get_dpdk_args(const struct ovsrec_open_vswitch *ovs_cfg, char ***argv)
> >> +{
> >> + struct dpdk_options_map {
> >> + const char *ovs_configuration;
> >> + const char *dpdk_option;
> >> + bool default_enabled;
> >> + const char *default_value;
> >> + } opts[] = {
> >> + {"dpdk_core_mask", "-c", true, "0x1"},
> >> + {"dpdk_mem_channels", "-n", true, "4"},
> >> + {"dpdk_alloc_mem", "-m", false, NULL},
> >> + {"dpdk_socket_mem", "--socket-mem", false, NULL},
> >
> > What do you think about adding a default for socket-mem?
>
> My only concern is I don't know what it should look like. This is the
> per-socket memory allocation and I'm not sure what kind of default makes
> sense, although with my specified defaults in the rest of the
> configuration perhaps "1024,0" is a 'proper' value.
>
> What do you think?
I think "1024,0" is reasonable default for users who don't want to know too much
low level config.
> <<snipped code>>
> >> static const struct option long_options[] = {
> >> {"help", no_argument, NULL, 'h'},
> >> @@ -166,7 +157,6 @@ parse_options(int argc, char *argv[], char
> >> **unixctl_pathp)
> >> {"bootstrap-ca-cert", required_argument, NULL,
> >> OPT_BOOTSTRAP_CA_CERT},
> >> {"enable-dummy", optional_argument, NULL, OPT_ENABLE_DUMMY},
> >> {"disable-system", no_argument, NULL, OPT_DISABLE_SYSTEM},
> >> - {"dpdk", required_argument, NULL, OPT_DPDK},
> >> {NULL, 0, NULL, 0},
> >> };
> >> char *short_options =
> >> ovs_cmdl_long_options_to_short_options(long_options);
> >> @@ -218,10 +208,6 @@ parse_options(int argc, char *argv[], char
> >> **unixctl_pathp)
> >> case '?':
> >> exit(EXIT_FAILURE);
> >>
> >> - case OPT_DPDK:
> >> - ovs_fatal(0, "--dpdk must be given at beginning of command
> >> line.");
> >
> > It might be helpful to leave the case statement here and call usage() or
> give
> > a deprecation message about the dpdk cmdline params for a release or two to
> > ensure a smooth transition to the new scheme - otherwise it'll probably end
> > up on the mailing list.
>
> I waffled on whether or not to change it to be "--dpdk is deprecated" or
> just remove it, but there's probably a user who will complain that we
> removed the DPDK feature. Okay, I'll update the NEWS file, and mention
> that the configuration of DPDK via command line is deprecated in favor
> of the db approach.
Yes, please don't put "--dpdk is deprecated" anywhere in the code base!!
It's a cleaner cut to code as you did, but worries me that a user will just
pull from the repo, run the their setup script and suddenly it won't run with
no error message to say why.
>
> Thanks for the review, Kevin!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
