Message ID | 1413225363-29660-1-git-send-email-max.suraev@fairwaves.co |
---|---|
State | Deferred |
Headers | show |
This patch is for OsmoTRX - I hope I've understood correctly message from http://openbsc.osmocom.org/trac/wiki/OsmoTRX about suitable ML. The 2nd part of the patch is monstrous 5Mb so it was rejected by mailman - this is due to removal of embedded sqlite (I don't believe the person who decided to include it was sober while making that decision :) I'll send it directly to maintainer to avoid spamming the list. 13.10.2014 20:36, Max пишет: > Signed-off-by: Max <max.suraev@fairwaves.co> > --- > .gitignore | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > create mode 100644 .gitignore > > diff --git a/.gitignore b/.gitignore > new file mode 100644 > index 0000000..0e0e704 > --- /dev/null > +++ b/.gitignore > @@ -0,0 +1,29 @@ > +.deps/ > +.libs/ > +*.lo > +*.la > +*.o > +osmo-trx > +CommonLibs/*Test > +Transceiver52M/osmo-trx-options.c > +Transceiver52M/osmo-trx-options.h > +Transceiver52M/osmo-trx.ggo~ > +aclocal.m4 > +autom4te.cache/ > +missing > +ltmain.sh > +install-sh > +depcomp > +configure > +config/ > +compile > +libtool > +stamp-h1 > +config.guess > +config.h.in > +config.status > +config.log > +config.h > +config.sub > +Makefile > +Makefile.in >
Max wrote: > +++ b/.gitignore .. > +CommonLibs/*Test > +Transceiver52M/osmo-trx-options.c > +Transceiver52M/osmo-trx-options.h > +Transceiver52M/osmo-trx.ggo~ Why is that needed? //Peter
The *Test are the binaries generated during the build - not sure we need them at all but they certainly do not belong to git. The options are generated as part of 2nd patch which was too big for the ML due to sqlite removal. This patch introduces command-line options handling via gengetopt which makes using sqlite db for configuration optional. 14.10.2014 02:50, Peter Stuge пишет: > Max wrote: >> +++ b/.gitignore > .. >> +CommonLibs/*Test >> +Transceiver52M/osmo-trx-options.c >> +Transceiver52M/osmo-trx-options.h >> +Transceiver52M/osmo-trx.ggo~ > > Why is that needed? > > > //Peter >
Hi Max, On Mon, Oct 13, 2014 at 09:01:50PM +0200, ☎ wrote: > Attached is a 2nd patch - it completely disables sqlite, all the configuration for > osmo-trx is supplied through command-line options. This makes troubleshooting easier > because now we have single source of truth: all the defaults are in one place. Thanks for looking into this. I agree that the sqlite configuration should be removed because it only causes confusion at this point. The original intention was to have backwards compatibility with OpenBTS-style sqlite options, but the number of users of that approach is approximately -1. Logging is the other concern. Have you tried removing the configuration table entirely? The existing queries are rarely, if ever, used. Without sqlite, the table shouldn't exist at all. Some general comments. Please remove code when you intend to remove it - committing commented out lines makes code and the patch difficult to read. Git will track the changes. Pull requests work well for large commits, such as sqlite3.c removal. > diff --git a/Transceiver52M/osmo-trx.cpp b/Transceiver52M/osmo-trx.cpp > index 9215fa5..16ac7b9 100644 > --- a/Transceiver52M/osmo-trx.cpp > +++ b/Transceiver52M/osmo-trx.cpp > - > +volatile bool gshutdown = false; // FIXME: what the ... is that for? The shutdown variable is a relic from OpenBTS, and exists because of the mess involving threads and shutdown. That issue is unrelated to logging or sqlite. > - refstr = config->extref ? "Enabled" : "Disabled"; > - fillstr = config->filler ? "Enabled" : "Disabled"; > - divstr = config->diversity ? "Enabled" : "Disabled"; > - > std::ostringstream ost(""); > ost << "Config Settings" << std::endl; > ost << " Log Level............... " << config->log_level << std::endl; > @@ -176,9 +96,9 @@ bool trx_setup_config(struct trx_config *config) > ost << " TRX Address............. " << config->addr << std::endl; > ost << " Channels................ " << config->chans << std::endl; > ost << " Samples-per-Symbol...... " << config->sps << std::endl; > - ost << " External Reference...... " << refstr << std::endl; > - ost << " C0 Filler Table......... " << fillstr << std::endl; > - ost << " Diversity............... " << divstr << std::endl; > + ost << " External Reference...... " << (config->extref ? "Enabled" : "Disabled") << std::endl; > + ost << " C0 Filler Table......... " << (config->filler ? "Enabled" : "Disabled") << std::endl; > + ost << " Diversity............... " << (config->diversity ? "Enabled" : "Disabled") << std::endl; > ost << " Tuning offset........... " << config->offset << std::endl; > std::cout << ost << std::endl; Please put whitespace / formatting changes in a separate patch. Osmocom projects generally use kernel style, which becomes somewhat odd and less strict in the context of C++. In this case, the code change is fine, but don't add it to an already massive patch. -TT
Thanks for review. I'll cleanup patches and make a pull-request to your repo on github. 14.10.2014 21:10, ttsou пишет: > Hi Max, > > On Mon, Oct 13, 2014 at 09:01:50PM +0200, ☎ wrote: >> Attached is a 2nd patch - it completely disables sqlite, all the configuration for >> osmo-trx is supplied through command-line options. This makes troubleshooting easier >> because now we have single source of truth: all the defaults are in one place. > > Thanks for looking into this. I agree that the sqlite configuration should be > removed because it only causes confusion at this point. The original intention > was to have backwards compatibility with OpenBTS-style sqlite options, but the > number of users of that approach is approximately -1. > > Logging is the other concern. Have you tried removing the configuration > table entirely? The existing queries are rarely, if ever, used. Without > sqlite, the table shouldn't exist at all. > > Some general comments. Please remove code when you intend to remove it - > committing commented out lines makes code and the patch difficult to read. > Git will track the changes. Pull requests work well for large commits, > such as sqlite3.c removal. > >> diff --git a/Transceiver52M/osmo-trx.cpp b/Transceiver52M/osmo-trx.cpp >> index 9215fa5..16ac7b9 100644 >> --- a/Transceiver52M/osmo-trx.cpp >> +++ b/Transceiver52M/osmo-trx.cpp >> - >> +volatile bool gshutdown = false; // FIXME: what the ... is that for? > > The shutdown variable is a relic from OpenBTS, and exists because of > the mess involving threads and shutdown. That issue is unrelated to logging or > sqlite. > >> - refstr = config->extref ? "Enabled" : "Disabled"; >> - fillstr = config->filler ? "Enabled" : "Disabled"; >> - divstr = config->diversity ? "Enabled" : "Disabled"; >> - >> std::ostringstream ost(""); >> ost << "Config Settings" << std::endl; >> ost << " Log Level............... " << config->log_level << std::endl; >> @@ -176,9 +96,9 @@ bool trx_setup_config(struct trx_config *config) >> ost << " TRX Address............. " << config->addr << std::endl; >> ost << " Channels................ " << config->chans << std::endl; >> ost << " Samples-per-Symbol...... " << config->sps << std::endl; >> - ost << " External Reference...... " << refstr << std::endl; >> - ost << " C0 Filler Table......... " << fillstr << std::endl; >> - ost << " Diversity............... " << divstr << std::endl; >> + ost << " External Reference...... " << (config->extref ? "Enabled" : "Disabled") << std::endl; >> + ost << " C0 Filler Table......... " << (config->filler ? "Enabled" : "Disabled") << std::endl; >> + ost << " Diversity............... " << (config->diversity ? "Enabled" : "Disabled") << std::endl; >> ost << " Tuning offset........... " << config->offset << std::endl; >> std::cout << ost << std::endl; > > Please put whitespace / formatting changes in a separate patch. Osmocom > projects generally use kernel style, which becomes somewhat odd and less > strict in the context of C++. In this case, the code change is fine, but don't > add it to an already massive patch. > > -TT >
diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..0e0e704 --- /dev/null +++ b/.gitignore @@ -0,0 +1,29 @@ +.deps/ +.libs/ +*.lo +*.la +*.o +osmo-trx +CommonLibs/*Test +Transceiver52M/osmo-trx-options.c +Transceiver52M/osmo-trx-options.h +Transceiver52M/osmo-trx.ggo~ +aclocal.m4 +autom4te.cache/ +missing +ltmain.sh +install-sh +depcomp +configure +config/ +compile +libtool +stamp-h1 +config.guess +config.h.in +config.status +config.log +config.h +config.sub +Makefile +Makefile.in
Signed-off-by: Max <max.suraev@fairwaves.co> --- .gitignore | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 .gitignore