Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a flat-file implementation of property_set / property_get. #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

owtaylor
Copy link

@owtaylor owtaylor commented Sep 9, 2016

For non-android systems, implement property_set() and property_get()
in terms of operations on a flat file of key=value pairs. The path is
hard-coded in include.mk to $sysconfdir/adb.properties.

The implementation is not particularly efficient, but designed to be
safe against updates from multiple threads and processes.

@@ -35,8 +35,13 @@ extern "C" {
** WARNING: system/bionic/include/sys/system_properties.h also defines
** these, but with different names. (TODO: fix that)
*/
#ifdef PROP_NAME_MAX
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You already have something similar in the CFLAGS. I would prefer to keep it only there, so as to require minimal changes to the original Android code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PROPERTY_NAME/VALUE_MAX have to be the same when properties.h is included as when the code is compiled; this is very different from PROPERTIES_PATH which only needs to be defined when compiling properties_nonandroid.c.

It would be possible, I suppose, to adjust the makefiles so that OPT_CFLAGS/OPT_CXXFLAGS are replaced with BASE_CFLAGS/BASE_CXXFLAGS and we add -D flags there, but I think that's really ugly.

What I replaced this with:

#if !ADB_NON_ANDROID
#include <sys/system_properties.h>
#else
+#define PROP_KEY_MAX 32
+#define PROP_VALUE_MAX 92
#endif

to keep this clearly in an existing non-android block?

@hadess
Copy link
Owner

hadess commented Sep 9, 2016

In addition to the changes mentioned, I'd really like to have 1) room in the implementation to define some properties programmatically (quite a few of them should be provided by systemd and co.) 2) have an example file with a list of properties with a nice default value, if only to show the syntax.

@owtaylor
Copy link
Author

owtaylor commented Sep 9, 2016

Do you have an example of something that we could implement programmatically that adbd needs? It seems like most of the ro.* it reads just don't matter for the non-android case. I believe that ro.*

Hmm, an example file is a little tricky because while the file is flat:

  • It doesn't support comments
  • It is ordered by key value
  • It gets updated by changing existing lines and inserting lines between existing lines

It's basically human readable but only human writable at an emergency. I suppose I can move to more human friendly:

  • Ignore initial whitespace, and lines beginning with #
  • Drop the ordering by key value - it allows future efficient bsearch lookups, but that doesn't matter
  • Insert all programmatically added lines at the end of the file

then we could ship an example adb.properties with a commented out service.adb.tcp.port and a comment about syntax.

@owtaylor
Copy link
Author

owtaylor commented Sep 9, 2016

"I believe that ro.*" - meant to say that the "ro." properties are android build time, as far as I can determine - system properties aren't used for dynamic system information.

Because top_builddir wasn't set, config.mk wasn't read for the
toplevel directory.
For non-android systems, implement property_set() and property_get()
in terms of operations on a flat file of key=value pairs. The path is
hard-coded in include.mk to $sysconfdir/adb.properties.

The implementation is not particularly efficient, but designed to be
safe against updates from multiple threads and processes.
@owtaylor
Copy link
Author

owtaylor commented Sep 9, 2016

Updated my patches to:

  • merge the PROP_* constants with the existing ADB_NON_ANDROID block
  • make the file format human editable (commnents, no including trailing whitespace into values, etc.)
  • install a sample file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants