2012-11-14 10:16:03

by Frederic Danis

[permalink] [raw]
Subject: [PATCH 1/4] adapter: Convert storage aliases

---
src/adapter.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 66 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index f9eb3af..ea7b8ff 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -86,6 +86,7 @@

#define SETTINGS_PATH STORAGEDIR "/%s/settings"
#define CACHE_PATH STORAGEDIR "/%s/cache/%s"
+#define DEVICE_INFO_PATH STORAGEDIR "/%s/%s/info"

static GSList *adapter_drivers = NULL;

@@ -2533,6 +2534,68 @@ static void convert_names_entry(char *key, char *value, void *user_data)
store_cached_name(&local, &peer, value);
}

+struct device_converter {
+ char *address;
+ void (*cb)(GKeyFile *key_file, void *value);
+};
+
+static void convert_aliases_entry(GKeyFile *key_file, void *value)
+{
+ g_key_file_set_string(key_file, "General", "Alias", value);
+}
+
+static void convert_entry(char *key, char *value, void *user_data)
+{
+ struct device_converter *converter = user_data;
+ char filename[PATH_MAX + 1];
+ GKeyFile *key_file;
+ char *data;
+ gsize length = 0;
+
+ if (key[17] == '#')
+ key[17] = '\0';
+
+ if (bachk(key) != 0)
+ return;
+
+ snprintf(filename, PATH_MAX, DEVICE_INFO_PATH, converter->address, key);
+ filename[PATH_MAX] = '\0';
+ create_file(filename, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+
+ key_file = g_key_file_new();
+ g_key_file_load_from_file(key_file, filename, 0, NULL);
+ converter->cb(key_file, value);
+
+ data = g_key_file_to_data(key_file, &length, NULL);
+ g_file_set_contents(filename, data, length, NULL);
+ g_free(data);
+
+ g_key_file_free(key_file);
+}
+
+static void convert_file(char *file, char *address,
+ void (*cb)(GKeyFile *key_file, void *value))
+{
+ char filename[PATH_MAX + 1];
+ struct device_converter converter;
+ char *str;
+
+ snprintf(filename, PATH_MAX, STORAGEDIR "/%s/%s", address, file);
+ filename[PATH_MAX] = '\0';
+
+ str = textfile_get(filename, "converted");
+ if (str && strcmp(str, "yes") == 0) {
+ DBG("Legacy file %s already converted", filename);
+ } else {
+ converter.address = address;
+ converter.cb = cb;
+
+ textfile_foreach(filename, convert_entry, &converter);
+ textfile_put(filename, "converted", "yes");
+ }
+ free(str);
+}
+
static void convert_device_storage(struct btd_adapter *adapter)
{
char filename[PATH_MAX + 1];
@@ -2553,6 +2616,9 @@ static void convert_device_storage(struct btd_adapter *adapter)
textfile_put(filename, "converted", "yes");
}
free(str);
+
+ /* Convert aliases */
+ convert_file("aliases", address, convert_aliases_entry);
}

static void convert_config(struct btd_adapter *adapter, const char *filename,
--
1.7.9.5



2012-11-14 11:11:40

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH 1/4] adapter: Convert storage aliases

Hi Frederic,

On Wed, Nov 14, 2012, Frederic Danis wrote:
> On 14/11/2012 11:45, Johan Hedberg wrote:
> >Hi Frederic,
> >
> >On Wed, Nov 14, 2012, Fr?d?ric Danis wrote:
> >> #define SETTINGS_PATH STORAGEDIR "/%s/settings"
> >> #define CACHE_PATH STORAGEDIR "/%s/cache/%s"
> >>+#define DEVICE_INFO_PATH STORAGEDIR "/%s/%s/info"
> >
> >Why do you have these separate defines? Do you need them in multiple
> >places? The define names are longer than the actual values and you're
> >just obfuscating the actual call that uses them (since you need to look
> >in two places to figure out of the format parameters are actually
> >matching the format string). So please don't use these.
> >
>
> CACHE_PATH and DEVICE_INFO_PATH are only used in one place, so they
> can be removed.
>
> SETTINGS_PATH is used in both store_adapter_info() and
> load_config(), so I think it is better to keep it.

I'd still just duplicate it in both places for readability. These
locations will be static at least for the entire 5.x series and maybe
longer.

Johan

2012-11-14 11:04:39

by Frederic Danis

[permalink] [raw]
Subject: Re: [PATCH 1/4] adapter: Convert storage aliases

Hello Johan,

On 14/11/2012 11:45, Johan Hedberg wrote:
> Hi Frederic,
>
> On Wed, Nov 14, 2012, Fr?d?ric Danis wrote:
>> #define SETTINGS_PATH STORAGEDIR "/%s/settings"
>> #define CACHE_PATH STORAGEDIR "/%s/cache/%s"
>> +#define DEVICE_INFO_PATH STORAGEDIR "/%s/%s/info"
>
> Why do you have these separate defines? Do you need them in multiple
> places? The define names are longer than the actual values and you're
> just obfuscating the actual call that uses them (since you need to look
> in two places to figure out of the format parameters are actually
> matching the format string). So please don't use these.
>

CACHE_PATH and DEVICE_INFO_PATH are only used in one place, so they can
be removed.

SETTINGS_PATH is used in both store_adapter_info() and load_config(), so
I think it is better to keep it.

Fred


--
Frederic Danis Open Source Technology Center
[email protected] Intel Corporation


2012-11-14 10:45:53

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH 1/4] adapter: Convert storage aliases

Hi Frederic,

On Wed, Nov 14, 2012, Fr?d?ric Danis wrote:
> #define SETTINGS_PATH STORAGEDIR "/%s/settings"
> #define CACHE_PATH STORAGEDIR "/%s/cache/%s"
> +#define DEVICE_INFO_PATH STORAGEDIR "/%s/%s/info"

Why do you have these separate defines? Do you need them in multiple
places? The define names are longer than the actual values and you're
just obfuscating the actual call that uses them (since you need to look
in two places to figure out of the format parameters are actually
matching the format string). So please don't use these.

Johan

2012-11-14 10:16:06

by Frederic Danis

[permalink] [raw]
Subject: [PATCH 4/4] device: Use new storage for device trust

As set_trust() changes value in device structure and write to
storage is deferred, property set could not return failure.
---
src/device.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/device.c b/src/device.c
index c508d18..792fe3d 100644
--- a/src/device.c
+++ b/src/device.c
@@ -226,6 +226,9 @@ static gboolean store_device_info_cb(gpointer user_data)
g_key_file_set_string(key_file, "General", "Alias",
device->alias);

+ g_key_file_set_boolean(key_file, "General", "Trusted",
+ device->trusted);
+
ba2str(adapter_get_address(device->adapter), adapter_addr);
ba2str(&device->bdaddr, device_addr);
snprintf(filename, PATH_MAX, INFO_PATH, adapter_addr, device_addr);
@@ -713,23 +716,14 @@ static gboolean dev_property_get_trusted(const GDBusPropertyTable *property,
static void set_trust(GDBusPendingPropertySet id, gboolean value, void *data)
{
struct btd_device *device = data;
- struct btd_adapter *adapter = device->adapter;
- char srcaddr[18], dstaddr[18];
- int err;

if (device->trusted == value)
return g_dbus_pending_property_success(id);

- ba2str(adapter_get_address(adapter), srcaddr);
- ba2str(&device->bdaddr, dstaddr);
-
- err = write_trust(srcaddr, dstaddr, device->bdaddr_type, value);
- if (err < 0)
- return g_dbus_pending_property_error(id,
- ERROR_INTERFACE ".Failed", strerror(-err));
-
device->trusted = value;

+ store_device_info(device);
+
g_dbus_emit_property_changed(btd_get_dbus_connection(),
device->path, DEVICE_INTERFACE, "Trusted");

@@ -1743,6 +1737,10 @@ static void load_info(struct btd_device *device, const gchar *local,
device->alias = g_key_file_get_string(key_file, "General", "Alias",
NULL);

+ /* Load trust */
+ device->trusted = g_key_file_get_boolean(key_file, "General",
+ "Trusted", NULL);
+
if (store_needed)
store_device_info(device);

@@ -1787,8 +1785,6 @@ struct btd_device *device_create(struct btd_adapter *adapter,

load_info(device, srcaddr, address);

- device->trusted = read_trust(src, address, device->bdaddr_type);
-
if (read_blocked(src, &device->bdaddr, device->bdaddr_type))
device_block(device, FALSE);

--
1.7.9.5


2012-11-14 10:16:05

by Frederic Danis

[permalink] [raw]
Subject: [PATCH 3/4] adapter: Convert storage trusts

All entry in trusts file are set to [all] (if a device is not trusted
it does not have entry in this file).
So, we do not need to check entry value and set device (entry key) as
trusted.
---
src/adapter.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index ea7b8ff..99d58b5 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2544,6 +2544,11 @@ static void convert_aliases_entry(GKeyFile *key_file, void *value)
g_key_file_set_string(key_file, "General", "Alias", value);
}

+static void convert_trusts_entry(GKeyFile *key_file, void *value)
+{
+ g_key_file_set_boolean(key_file, "General", "Trusted", TRUE);
+}
+
static void convert_entry(char *key, char *value, void *user_data)
{
struct device_converter *converter = user_data;
@@ -2619,6 +2624,9 @@ static void convert_device_storage(struct btd_adapter *adapter)

/* Convert aliases */
convert_file("aliases", address, convert_aliases_entry);
+
+ /* Convert trusts */
+ convert_file("trusts", address, convert_trusts_entry);
}

static void convert_config(struct btd_adapter *adapter, const char *filename,
--
1.7.9.5


2012-11-14 10:16:04

by Frederic Danis

[permalink] [raw]
Subject: [PATCH 2/4] device: Use new storage for device alias

As set_alias() changes value in device structure and write to
storage is deferred, property set could not return failure.
---
src/device.c | 28 +++++++++++-----------------
1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/src/device.c b/src/device.c
index 48970d2..c508d18 100644
--- a/src/device.c
+++ b/src/device.c
@@ -222,6 +222,10 @@ static gboolean store_device_info_cb(gpointer user_data)

g_key_file_set_string(key_file, "General", "Name", device->name);

+ if (device->alias != NULL)
+ g_key_file_set_string(key_file, "General", "Alias",
+ device->alias);
+
ba2str(adapter_get_address(device->adapter), adapter_addr);
ba2str(&device->bdaddr, device_addr);
snprintf(filename, PATH_MAX, INFO_PATH, adapter_addr, device_addr);
@@ -432,28 +436,17 @@ static void set_alias(GDBusPendingPropertySet id, const char *alias,
void *data)
{
struct btd_device *device = data;
- struct btd_adapter *adapter = device->adapter;
- char srcaddr[18], dstaddr[18];
- int err;

/* No change */
if ((device->alias == NULL && g_str_equal(alias, "")) ||
g_strcmp0(device->alias, alias) == 0)
return g_dbus_pending_property_success(id);

- ba2str(adapter_get_address(adapter), srcaddr);
- ba2str(&device->bdaddr, dstaddr);
-
- /* Remove alias if empty string */
- err = write_device_alias(srcaddr, dstaddr, device->bdaddr_type,
- g_str_equal(alias, "") ? NULL : alias);
- if (err < 0)
- return g_dbus_pending_property_error(id,
- ERROR_INTERFACE ".Failed", strerror(-err));
-
g_free(device->alias);
device->alias = g_str_equal(alias, "") ? NULL : g_strdup(alias);

+ store_device_info(device);
+
g_dbus_emit_property_changed(btd_get_dbus_connection(),
device->path, DEVICE_INTERFACE, "Alias");

@@ -1746,6 +1739,10 @@ static void load_info(struct btd_device *device, const gchar *local,
g_free(str);
}

+ /* Load alias */
+ device->alias = g_key_file_get_string(key_file, "General", "Alias",
+ NULL);
+
if (store_needed)
store_device_info(device);

@@ -1759,7 +1756,7 @@ struct btd_device *device_create(struct btd_adapter *adapter,
struct btd_device *device;
const gchar *adapter_path = adapter_get_path(adapter);
const bdaddr_t *src;
- char srcaddr[18], alias[MAX_NAME_LENGTH + 1];
+ char srcaddr[18];
uint16_t vendor, product, version;

device = g_try_malloc0(sizeof(struct btd_device));
@@ -1790,9 +1787,6 @@ struct btd_device *device_create(struct btd_adapter *adapter,

load_info(device, srcaddr, address);

- if (read_device_alias(srcaddr, address, bdaddr_type, alias,
- sizeof(alias)) == 0)
- device->alias = g_strdup(alias);
device->trusted = read_trust(src, address, device->bdaddr_type);

if (read_blocked(src, &device->bdaddr, device->bdaddr_type))
--
1.7.9.5