Following my previous email where I outlined my comments, here are specific comments on your patch (sorry about the long delay)
On Fri, Jan 25, 2019 at 6:27 PM Brian Ledbetter <[email protected]> wrote: > Here's my first stab at solving this problem in a backwards-compatible > way, let me know what you think. I've added a --resolv flag to the loader > to enable this behavior. > > patch-dhcp.cc: > > --- ./core/dhcp.cc~ 2019-01-25 11:18:29.173860626 -0500 > +++ ./core/dhcp.cc 2019-01-25 11:17:19.546083373 -0500 > @@ -29,6 +29,12 @@ > #include <libc/network/__dns.hh> > > using namespace boost::asio; > +bool generate_resolv_conf = false; > + > +void enable_resolv_conf() > +{ > + generate_resolv_conf = true; > +} > I don't think we need this flag, we can always write this file (if we can), I think the performance penalty is trivial. This will cut this patch size in half, and will avoid frustrations for future users who won't even know this option exist, and not get /etc/resolv.conf and wonder again why things don't work. > > dhcp::dhcp_worker net_dhcp_worker; > > @@ -720,6 +726,31 @@ > }); > > osv::set_dns_config(dm.get_dns_ips(), > std::vector<std::string>()); > + > + // Some linux applications (go, erlang) depend on > /etc/resolv.conf to be > + // populated with DNS server information in order to function. > The --resolv > + // option has been added to the kernel loader, which will > enable this > + // feature. > + // > + // TODO: It's possible that the /etc directory does not exist > at this time. > + // We should ideally check and create it if necessary. What's > the right > + // way to do that? > Why is it possible that /etc doesn't exist yet? I think we start DHCP *after* mounting the root filesystem. I have also suggested in my other email to have (in usr_rofs.manifest.skel) a symlink from /etc/resolv.conf to /tmp/resolv.conf, so it will be writable even on read-only filesystems. I'm not an expert on the new rofs setup, Waldek can comment if that makes sense. + // > + // TODO: We are currently ignoring DHCP-provided default domain > + // names. I'll have to dig further into the code to see if I > can ingest > + // those. > + // > + if( generate_resolv_conf ) > Please use - here and below - the OSv coding style. We put a space after the "if" but not after the ( or before the ), and the "{" is on the same line. You can see examples in dhcp.cc and in other source files. There is also an incomplete CODINGSTYLE.md. + { > + FILE *f = fopen("/etc/resolv.conf", "w+"); > Why "w+"? "w+" is for when you will also need to read from the same file. Here you just plan to truncate and write to it. So a normal "w" is exactly what you need. + if( f ) { > + fprintf( f, "## Autoconfigured by DHCP\n" ); > + for( auto &ip : dm.get_dns_ips() ) > + { > + fprintf( f, "server %s\n", > ip.to_string().c_str() ); > + dhcp_i( "Added DNS server: %s\n", > ip.to_string().c_str() ); > + } > + fclose( f ); > + dhcp_i( "Generated /etc/resolv.conf" ); > + } > + } > + > if (dm.get_hostname().size()) { > sethostname(dm.get_hostname().c_str(), > dm.get_hostname().size()); > dhcp_i("Set hostname to: %s", dm.get_hostname().c_str()); > > > patch-loader.cc: > > --- ./loader.cc~ 2019-01-25 11:10:40.091359718 -0500 > +++ ./loader.cc 2019-01-25 11:11:40.243167695 -0500 > @@ -144,10 +144,13 @@ > bool opt_assign_net = false; > bool opt_maxnic = false; > int maxnic; > +static bool opt_resolv_conf = false; > > static int sampler_frequency; > static bool opt_enable_sampler = false; > > +void enable_resolv_conf(); > + > void parse_options(int loader_argc, char** loader_argv) > { > namespace bpo = boost::program_options; > @@ -182,6 +185,7 @@ > ("delay", bpo::value<float>()->default_value(0), "delay in > seconds before boot") > ("redirect", bpo::value<std::string>(), "redirect stdout and > stderr to file") > ("disable_rofs_cache", "disable ROFS memory cache") > + ("resolv", "generate an /etc/resolv.conf file with DHCP DNS > values (for Linux compatibility)") > ; > bpo::variables_map vars; > // don't allow --foo bar (require --foo=bar) so we can find the first > non-option > @@ -234,6 +238,11 @@ > enable_verbose(); > } > > + if (vars.count("resolv")) { > + opt_resolv_conf = true; > + enable_resolv_conf(); > + } > + > if (vars.count("sampler")) { > sampler_frequency = vars["sampler"].as<int>(); > opt_enable_sampler = true; > > > patch-run.py: > > --- ./scripts/run.py~ 2019-01-25 11:14:20.858654652 -0500 > +++ ./scripts/run.py 2019-01-25 11:14:52.406553829 -0500 > @@ -43,6 +43,8 @@ > execute = cmdline.read() > if options.verbose: > execute = "--verbose " + execute > + if options.resolv: > + execute = "--resolv " + execute > > if options.jvm_debug or options.jvm_suspend: > if '-agentlib:jdwp' in execute: > @@ -461,6 +463,8 @@ > help="Enable graphics mode.") > parser.add_argument("-V", "--verbose", action="store_true", > help="pass --verbose to OSv, to display more > debugging information on the console") > + parser.add_argument("--resolv", action="store_true", > + help="pass --resolv to OSv, to generate an > /etc/resolv.conf file for Linux compatibility") > parser.add_argument("--forward", metavar="RULE", action="append", > default=[], > help="add network forwarding RULE (QEMU syntax)") > parser.add_argument("--dry-run", action="store_true", > > > -- > You received this message because you are subscribed to the Google Groups > "OSv Development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
