GNOME Bugzilla – Bug 734888
GLib has no helper functions to work with W32 Registry
Last modified: 2015-06-05 23:14:33 UTC
This doesn't hurt much when you only need to get a single value or enumerate subkeys of a single key, but anything beyond that is just unbearable. I would really like to have these, the make fixing bug 666831 much easier.
Created attachment 283570 [details] [review] Add W32 Registry reading API to gio
Created attachment 283571 [details] [review] Add W32 Registry reading API to gio v2: Fixed includes in the header
Typically, windows-specific API use "win32" and not "w32" (no match in existing APIs)
Review of attachment 283571 [details] [review]: Here is a first round of comments. It is very cool to see something like this happening. ::: gio/gw32registrykey.c @@ +1,3 @@ +/* GIO - GLib Input, Output and Streaming Library + * + * Copyright (C) 2006-2007 Red Hat, Inc. I guess you are not working for red hat. @@ +94,3 @@ + **/ + +struct _GW32RegistryKeyPrivate { Let's use GWin32 @@ +138,3 @@ + priv = self->priv; + + if (priv) priv is always there @@ +140,3 @@ + if (priv) + { + g_clear_object (&priv->ancestor); g_clear_object stuff should go into dispose @@ +150,3 @@ + } + + if (G_OBJECT_CLASS (g_w32_registry_key_parent_class)->finalize) no need for this check, finalize is ensured to be there. @@ +157,3 @@ + * g_w32_registry_key_open: + * @path: absolute full name of a key to open (in UTF-8) + * @error: a pointer to a %NULL #GError, or %NULL missing g-i annotation @@ +182,3 @@ + GW32RegistryKey *result = NULL; + + g_return_val_if_fail (error == NULL || *error == NULL, NULL); I guess path needs to also be checked? @@ +263,3 @@ + else + { + g_error ("Root key '%S' is not a pre-defined key", first_chunk); I doubt we want to use g_error, I guess a g_warning instead? @@ +276,3 @@ + if (second_chunk_begin != first_chunk_end && second_chunk_begin[0] == L'\0') + { + g_error ("Key name '%S' ends with '\\'", path); same here @@ +304,3 @@ + result->priv->ancestor = NULL; + result->priv->handle = key_handle; + result->priv->absolute_path_w = keep it on the same line and split on params @@ +316,3 @@ + * @key: a #GW32RegistryKey to use as a starting point + * @subkey: name of a key to open (in UTF-8), relative to @key + * @error: a pointer to a %NULL #GError, or %NULL missing annotation @@ +331,3 @@ + GW32RegistryKey *result = NULL; + + g_return_val_if_fail (subkey != NULL, NULL); what about key? this should check for G_IS_WIN32_REGISTRY_KEY @@ +349,3 @@ + * @key: a #GW32RegistryKey to use as a starting point + * @subkey: name of a key to open (in UTF-8), relative to @key + * @error: a pointer to a %NULL #GError, or %NULL missing annotation @@ +370,3 @@ + const gunichar2 *key_path; + + g_return_val_if_fail (key != NULL, NULL); use G_IS_WIN32_REGISTRY_KEY @@ +388,3 @@ + if (end_of_subkey[0] == L'\\') + { + g_error ("Subkey name '%S' ends with '\\'", subkey); g_warning? @@ +414,3 @@ + } + + result = G_W32_REGISTRY_KEY (g_object_new (g_w32_registry_key_get_type (), NULL)); this casting should not be needed @@ +417,3 @@ + + result->priv->handle = key_handle; + result->priv->absolute_path_w = same as before @@ +435,3 @@ +} + +struct _g_w32_foreach_key_utf8_helper_closure { { in the next line and I'd say typedef it so it is easier to read @@ +470,3 @@ + * @callback: a #GW32RegistryKeyForeachSubkeyFunc to call for each subkey + * @callback_data: (closure): user data for @callback + * @error: a pointer to %NULL #GError, or %NULL annotation? @@ +497,3 @@ + * @callback: a #GW32RegistryKeyForeachSubkeyWFunc to call for each subkey + * @callback_data: (closure): user data for @callback + * @error: a pointer to %NULL #GError, or %NULL annotation? @@ +518,3 @@ + GError *callback_error = NULL; + + g_return_val_if_fail (key != NULL, FALSE); G_IS_WIN32_REGISTRY_KEY @@ +563,3 @@ + */ + if (callback_error) + g_error_free (callback_error); use g_clear_error @@ +581,3 @@ +} + +struct _g_w32_foreach_value_utf8_helper_closure { typedef and new line for { @@ +604,3 @@ + + value_name_u8 = g_utf16_to_utf8 (value_name, value_name_len, NULL, + &value_name_u8_len, error); wrong alignment @@ +614,3 @@ + { + value_data_u8 = g_convert ((const gchar *) value_data, + value_data_size - sizeof (gunichar2) /* excl. 0 */, align to the ( @@ +656,3 @@ + * @callback: a #GW32RegistryKeyForeachValueFunc to call for each value + * @callback_data: (closure): user data for @callback + * missing error @@ +659,3 @@ + * Call @callback for each value of the @key. + * + * Returns: TRUE if no errors occurred, FALSE otherwise. missing % for the TRUE and FALSE @@ +720,3 @@ + * @callback: a #GW32RegistryKeyForeachValueWFunc to call for each value + * @callback_data: (closure): user data for @callback + * missing error @@ +724,3 @@ + * in UTF-16 encoding. + * + * Returns: TRUE if no errors occurred, FALSE otherwise. same as before missing % @@ +745,3 @@ + GError *callback_error = NULL; + + g_return_val_if_fail (key != NULL, FALSE); G_IS_WIN32_REGISTRY_KEY @@ +752,3 @@ + NULL, NULL, NULL, + NULL, NULL, + NULL, &values_count, &max_value_name_len, &max_value_data_len, NULL, NULL); splice a bit more this in a couple of more lines @@ +837,3 @@ + */ + if (callback_error) + g_error_free (callback_error); use g_clear_error? @@ +857,3 @@ + +static void +_g_w32_registry_key_reread_kernel (GW32RegistryKey *key, GW32RegistryKeyPrivate *buf) split params @@ +895,3 @@ + ((char *) &basic_info)[datasize + 1] = 0; + + buf->absolute_path_w = g_memdup (&basic_info->Name[0], basic_info->NameLength + sizeof (gunichar2)); split params @@ +900,3 @@ + +static void +_g_w32_registry_key_reread_user (GW32RegistryKey *key, GW32RegistryKeyPrivate *buf) split params @@ +909,3 @@ + +static void +_g_w32_registry_key_reread (GW32RegistryKey *key, GW32RegistryKeyPrivate *buf) ditto @@ +968,3 @@ +const gchar * +g_w32_registry_key_get_path (GW32RegistryKey *key) +{ check the key with g_return_val_if_fail @@ +998,3 @@ +const gunichar2 * +g_w32_registry_key_get_path_w (GW32RegistryKey *key) +{ same here the return_val @@ +1001,3 @@ + if (WaitForSingleObject (key->priv->change_event, 0) == WAIT_OBJECT_0) + _g_w32_registry_key_update_path (key); + return key->priv->absolute_path_w; leave an empty line before the return @@ +1013,3 @@ + * @value_data_size: (out) (optional): size of the buffer pointed + * by @value_data. + * @error: a pointer to %NULL #GError, or %NULL annotation @@ +1036,3 @@ + gboolean result; + + g_return_val_if_fail (key != NULL, FALSE); do the right check @@ +1113,3 @@ + * @value_data_size: (out) (optional): size of the buffer pointed + * by @value_data. + * @error: a pointer to %NULL #GError, or %NULL annotation @@ +1126,3 @@ + * too. + * + * Returns: TRUE on success, FALSE on failure. missing % @@ +1145,3 @@ + DWORD req_value_data_size2; + + g_return_val_if_fail (key != NULL, FALSE); use the right check @@ +1256,3 @@ +} + +struct _g_w32_registry_walk_context { typedef @@ +1300,3 @@ + + if (error != NULL && *error != NULL) + g_clear_error (error); g_clear_error checks already for the passed in value @@ +1321,3 @@ + gchar *child_name; + const gchar *key_name_const = g_w32_registry_key_get_path (key); + child = g_w32_registry_key_open_subkey (key, subkey_name, error); empty line before assign the child @@ +1343,3 @@ + + if (error != NULL && *error != NULL) + g_clear_error (error); same here for the error @@ +1379,3 @@ + if (callback_w) + for (i = 0; result && i < count; i++) + result = same line @@ +1411,3 @@ + * @callback: a GW32RegistryKeyWalkWFunc to call for each subkey + * @callback_data: (closure): user data for @callback + * @error: a pointer to %NULL #GError, or %NULL annoatation @@ +1416,3 @@ + * the @callback for each subkey. The keys are walked breadth-first. + * + * Returns: TRUE if no errors occurred, FALSE otherwise. missing % @@ +1424,3 @@ + gpointer callback_data, + GError **error) +{ you need to g_return_if... @@ +1449,3 @@ + gpointer callback_data, + GError **error) +{ you need to g_return_if... or add it to the impl method @@ +1457,3 @@ +g_w32_registry_key_class_init (GW32RegistryKeyClass *klass) +{ + g_w32_registry_key_parent_class = g_type_class_peek_parent (klass); no need for this @@ +1459,3 @@ + g_w32_registry_key_parent_class = g_type_class_peek_parent (klass); + + G_OBJECT_CLASS (klass)->finalize = g_w32_registry_key_finalize; keep a member like in other objects @@ +1465,3 @@ +g_w32_registry_key_init (GW32RegistryKey *self) +{ + self->priv = (GW32RegistryKeyPrivate *) g_new0 (GW32RegistryKeyPrivate, 1); no need for this, just use g_win32_registry_key_get_instance_private(self) ::: gio/gw32registrykey.h @@ +1,3 @@ +/* GIO - GLib Input, Output and Streaming Library + * + * Copyright (C) 2006-2007 Red Hat, Inc. put the right copyright @@ +16,3 @@ + * Public License along with this library; if not, see <http://www.gnu.org/licenses/>. + * + * Author: Vlad Grecescu <b100dian@gmail.com> I guess this is not you @@ +59,3 @@ +typedef struct _GW32RegistryKey GW32RegistryKey; +typedef struct _GW32RegistryKeyClass GW32RegistryKeyClass; +typedef struct _GW32RegistryKeyPrivate GW32RegistryKeyPrivate; no need for this typedef here you can actually use _getinstance_private @@ +65,3 @@ + + /*< private >*/ + GW32RegistryKeyPrivate * priv; do not add this priv here. @@ +71,3 @@ + GObjectClass parent_class; + + /* Signals */ if you do not use this stuff just remove the comments @@ +79,3 @@ +}; + +GLIB_AVAILABLE_IN_2_42 from here one I see several issues: - params are not correctly aligned - there is some docs that are also in the c files so theys hould be removed
(In reply to comment #4) > Review of attachment 283571 [details] [review]: > > Here is a first round of comments. It is very cool to see something like this > happening. > > ::: gio/gw32registrykey.c > @@ +94,3 @@ > + **/ > + > +struct _GW32RegistryKeyPrivate { > > Let's use GWin32 I was afraid something like this would happen :( > @@ +263,3 @@ > + else > + { > + g_error ("Root key '%S' is not a pre-defined key", first_chunk); > > I doubt we want to use g_error, I guess a g_warning instead? No. Unless first component is a known key name, we can't open the key (actually, Windows Registry is said to only have 2 real root keys, all other root keys are aliases, but i've never had to use those). If this function is called with wrong key path, it's obviously an error on caller's part. > @@ +276,3 @@ > + if (second_chunk_begin != first_chunk_end && second_chunk_begin[0] == L'\0') > + { > + g_error ("Key name '%S' ends with '\\'", path); > > same here Same here. Wrong argument supplied to the function -> caller's fault. AFAIU the philosophy is "if the error is in your code or the functions you call, report via GError; if the error is on caller's part, report via g_error". g_warning is when something's fishy, but still OK to proceed. > @@ +388,3 @@ > + if (end_of_subkey[0] == L'\\') > + { > + g_error ("Subkey name '%S' ends with '\\'", subkey); > > g_warning? See above. > @@ +59,3 @@ > +typedef struct _GW32RegistryKey GW32RegistryKey; > +typedef struct _GW32RegistryKeyClass GW32RegistryKeyClass; > +typedef struct _GW32RegistryKeyPrivate GW32RegistryKeyPrivate; > > no need for this typedef here you can actually use _getinstance_private How? > @@ +65,3 @@ > + > + /*< private >*/ > + GW32RegistryKeyPrivate * priv; > > do not add this priv here. Where then?
About the priv structure, it seems that in the rest of the classes the keep the priv as a helper so I guess we can do the same. So basically in the _init method instead of allocating it by yourself you just need to call: key->priv = g_win32_registry_key_get_instance_private(key); About the g_error stuff, I am not so sure we want to abort the application just because something wrong is going on on the registry. We might want to check what it happens with i.e gsettings.
Created attachment 284831 [details] [review] Add W32 Registry reading API to gio v2: changed all enumerators to iterators, changed tree watching, style fixes
Review of attachment 284831 [details] [review]: Another minor review. Please fix it and then I'll try to actually undesrtand the code :) ::: gio/gwin32registrykey.c @@ +1,3 @@ +/* GIO - GLib Input, Output and Streaming Library + * + * Copyright (C) 2014 LRN as in the header @@ +108,3 @@ +#define G_WIN32_KEY_UNKNOWN -1 + +typedef enum { { in new line? @@ +196,3 @@ +G_DEFINE_TYPE_WITH_PRIVATE (GWin32RegistryKey, g_win32_registry_key, G_TYPE_OBJECT) + +static void g_win32_registry_key_dispose (GObject *base); why this prototype here if it is implement a couple of lines down? @@ +199,3 @@ + +static void +g_win32_registry_key_dispose (GObject *base) change base to object for consistency @@ +211,3 @@ + + if (!priv->predefined) + RegCloseKey (priv->handle); it is a dispose so this can run several times. you should set predefined to false I guess @@ +353,3 @@ + result->priv->ancestor = NULL; + result->priv->handle = key_handle; + result->priv->absolute_path_w = assing in the same line, then feel free to break in the params @@ +380,3 @@ + GWin32RegistryKey *result = NULL; + + g_return_val_if_fail (key != NULL, NULL); G_IS_WIN32_REGISTRY_KEY @@ +418,3 @@ + const gunichar2 *key_path; + + g_return_val_if_fail (key != NULL, NULL); same here @@ +500,3 @@ + + g_return_val_if_fail (iter != NULL, FALSE); + g_return_val_if_fail (key != NULL, FALSE); same here @@ +543,3 @@ + g_clear_object (&iter->key); +} + one single line to separate methods @@ +759,3 @@ + + g_return_val_if_fail (iter != NULL, FALSE); + g_return_val_if_fail (key != NULL, FALSE); same here @@ +818,3 @@ +} + + 2 lines -> 1 line @@ +1699,3 @@ + return TRUE; +} + 2 lines @@ +1756,3 @@ + PIO_STATUS_BLOCK status_block; + + g_return_val_if_fail (key != NULL, FALSE); use the right check @@ +1758,3 @@ + g_return_val_if_fail (key != NULL, FALSE); + + filter = assign in first line @@ +1846,3 @@ +g_win32_registry_key_erase_change_indicator (GWin32RegistryKey *key) +{ + g_return_if_fail (key != NULL); right check @@ +1867,3 @@ + gint changed; + + g_return_val_if_fail (key != NULL, FALSE); right check @@ +1883,3 @@ + +static void +g_win32_registry_key_init (GWin32RegistryKey *self) I'd use key instead of self ::: gio/gwin32registrykey.h @@ +1,3 @@ +/* GIO - GLib Input, Output and Streaming Library + * + * Copyright (C) 2014 LRN please put your real name and an email @@ +21,3 @@ + +#include <gio/gio.h> + I think here is missing some check to not directly include this file @@ +51,3 @@ +#endif + G_WIN32_REGISTRY_STR = 8 +} GWin32RegistryValueType; if this is ValueType, should all the enums have VALUE? @@ +66,3 @@ +typedef struct _GWin32RegistryValueIter GWin32RegistryValueIter; + +struct _GWin32RegistryKey { I think the { should be in a new line? please check the other code @@ +77,3 @@ +}; + +struct _GWin32RegistrySubkeyIter { this should be just a typdef in the header and have this in the cpp? also should it be a boxed? @@ +89,3 @@ +}; + +struct _GWin32RegistryValueIter { same here @@ +107,3 @@ + +GLIB_AVAILABLE_IN_2_42 +GType g_win32_registry_key_get_type (void); correct align with the other methods?
(In reply to comment #8) > Review of attachment 284831 [details] [review]: > > Another minor review. Please fix it and then I'll try to actually undesrtand > the code :) > > ::: gio/gwin32registrykey.h > @@ +66,3 @@ > +typedef struct _GWin32RegistryValueIter GWin32RegistryValueIter; > + > +struct _GWin32RegistryKey { > > I think the { should be in a new line? please check the other code No, it should not be. The other code doesn't do that. My guess is that this makes it easier to find struct definitions (grep for "struct <name> {"; without the "{" you'll have difficult time finding it). > @@ +77,3 @@ > +}; > + > +struct _GWin32RegistrySubkeyIter { > > this should be just a typdef in the header and have this in the cpp? also > should it be a boxed? > > @@ +89,3 @@ > +}; > + > +struct _GWin32RegistryValueIter { > > same here These are allocated on the stack, so the compiler has to know their size. Not sure about the "boxed" remark.
Created attachment 284863 [details] [review] Add W32 Registry reading API to gio v2: changed all enumerators to iterators, changed tree watching, style fixes v3: fixed most of the nit
Created attachment 284879 [details] [review] Add W32 Registry reading API to gio v2: changed all enumerators to iterators, changed tree watching, style fixes v3: fixed most of the nit v4: fixed character encoding in the header (d'oh!), fixed a typo
Review of attachment 284879 [details] [review]: just a few remarks ::: gio/gwin32registrykey.c @@ +1490,3 @@ + * @key: (in) (transfer none): a #GWin32RegistryKey + * @auto_expand: (in) TRUE to automatically expand G_WIN32_REGISTRY_VALUE_EXPAND_STR + * to G_WIN32_REGISTRY_VALUE_STR. Why not provide a seperate function for expand? I don't think it needs to be mixed in this call. @@ +1494,3 @@ + * Empty string means the '(Default)' value. + * @value_type: (out) (optional): type of the value retrieved. + * @value_data: (out callee-allocates) (optional): contents of the value. In general, when using the windows registry, you know the type of data you are querying, so you typically don't need to allocate. As it seems you have concerns about efficiency (by providing utf8 and utf16 versions), I think a static query function should be provided. @@ +1714,3 @@ + +/** + * g_win32_registry_key_watch_for_changes: I would just call it watch or watch_add (for_changes seems superfluous) @@ +1730,3 @@ + * g_win32_registry_key_has_changed() will return %TRUE until a call to + * g_win32_registry_key_erase_change_indicator() is made or the @key is put + * under watch by calling g_win32_registry_key_watch_for_changes() again. This seems a bit awkward to me. So if I want to watch for changes I will have to regularly poll has_changed()? Why not use a callback, as it is more usual? ::: gio/gwin32registrykey.h @@ +58,3 @@ + +typedef enum { + G_WIN32_REGISTRY_CHANGE_NAME = 1, More idiomatic for flags in glib is to use (1<<n) notation @@ +62,3 @@ + G_WIN32_REGISTRY_CHANGE_VALUES = 4, + G_WIN32_REGISTRY_CHANGE_SECURITY = 8, +} GWin32RegistryKeyChangeFlag; I think "watcher" could be more appropriate than "change" here (similar to other "watch" flags, or GBusNameWatcherFlags..)
(In reply to comment #12) > Review of attachment 284879 [details] [review]: > > just a few remarks > > ::: gio/gwin32registrykey.c > @@ +1490,3 @@ > + * @key: (in) (transfer none): a #GWin32RegistryKey > + * @auto_expand: (in) TRUE to automatically expand > G_WIN32_REGISTRY_VALUE_EXPAND_STR > + * to G_WIN32_REGISTRY_VALUE_STR. > > Why not provide a seperate function for expand? I don't think it needs to be > mixed in this call. I considered that. Turns out, i need to autoexpand 100% of the time (for the client code i wrote). Since this function outputs data into an argument output variable, not as a function return, you can't chain it functional-style either. So you'd have to call get_value(), check that it didn't fail, then either check for value type and call expand() (passing the value and a pointer to the variable where new size will be stored), or just call expand() and provide the type to it (it will do nothing if type is not EXPAND_STR). Much easier to just add one more argument to get_value(). That said, the implementation of expanding itself may go into its own internal function, which will be called by get_value() and get_next() to reduce code duplication. > > @@ +1494,3 @@ > + * Empty string means the '(Default)' value. > + * @value_type: (out) (optional): type of the value retrieved. > + * @value_data: (out callee-allocates) (optional): contents of the value. > > In general, when using the windows registry, you know the type of data you are > querying, so you typically don't need to allocate. > > As it seems you have concerns about efficiency (by providing utf8 and utf16 > versions), I think a static query function should be provided. True, caller-allocates (with a stack buffer) would be more efficient for values for which the size is known. However, the client code i wrote nearly always worked with strings, and strings need to be callee-allocated. I will consider adding a different version of get_value() that accepts caller-allocated buffer and fails if it's too small. For get_next() that still seems like a bad idea, as the implementation can internally allocate a buffer once, then re-use it for each value of a key. > > @@ +1714,3 @@ > + > +/** > + * g_win32_registry_key_watch_for_changes: > > I would just call it watch or watch_add (for_changes seems superfluous) OK. > > @@ +1730,3 @@ > + * g_win32_registry_key_has_changed() will return %TRUE until a call to > + * g_win32_registry_key_erase_change_indicator() is made or the @key is put > + * under watch by calling g_win32_registry_key_watch_for_changes() again. > > This seems a bit awkward to me. So if I want to watch for changes I will have > to regularly poll has_changed()? Why not use a callback, as it is more usual? For the client code i wrote indicator-based mechanism is *much* more efficient. Every time client code gets cached registry data, it checks the indicator (which is very cheap, as it's just an atomic get), and re-reads the data if anything changed. I think i can provide a separate variant that does invoke a user-provided callback (using the same APC mechanism, only APC is user-implemented), if that's what you would want. > > ::: gio/gwin32registrykey.h > @@ +58,3 @@ > + > +typedef enum { > + G_WIN32_REGISTRY_CHANGE_NAME = 1, > > More idiomatic for flags in glib is to use (1<<n) notation > > @@ +62,3 @@ > + G_WIN32_REGISTRY_CHANGE_VALUES = 4, > + G_WIN32_REGISTRY_CHANGE_SECURITY = 8, > +} GWin32RegistryKeyChangeFlag; > > I think "watcher" could be more appropriate than "change" here (similar to > other "watch" flags, or GBusNameWatcherFlags..) OK
Created attachment 285045 [details] [review] Add W32 Registry reading API to gio v2: changed all enumerators to iterators, changed tree watching, style fixes v3: fixed most of the nit v4: fixed character encoding in the header (d'oh!), fixed a typo v5: - g_win32_registry_key_watch{_for_changes,} - optional callback for g_win32_registry_key_watch - boxed types for iterators (no idea if they work; all i know is that the code compiles. - minor docs clarification - CHANGE -> WATCH - removed "Only <gio/gio.h> can be included directly" warning
Review of attachment 285045 [details] [review]: more nitpicks ::: gio/gwin32registrykey.c @@ +132,3 @@ + g_return_val_if_fail (iter != NULL, NULL); + + new_iter = g_new (GWin32RegistrySubkeyIter, 1); use g_new0 just in case @@ +207,3 @@ + **/ +GWin32RegistryValueIter * +g_win32_registry_value_iter_copy (const GWin32RegistryValueIter *iter) wrong spacing before ( @@ +213,3 @@ + g_return_val_if_fail (iter != NULL, NULL); + + new_iter = g_new (GWin32RegistryValueIter, 1); use g_new0 just in case
Review of attachment 285045 [details] [review]: See another round of reviews, it has some comments and questions inline. ::: gio/gwin32registrykey.c @@ +121,3 @@ + * state of the iterator is duplicated too. + * + * Returns: a copy of the @iter, free with g_win32_registry_subkey_iter_free () transfer full ? @@ +248,3 @@ + **/ +void +g_win32_registry_value_iter_free (GWin32RegistryValueIter *iter) one space before ( and before *iter @@ +332,3 @@ + + /* Full absolute path of the key, in UTF-16. Always allocated. + * Can become out of sync if the key is renamed from under us, from under us -> under our feet? @@ +368,3 @@ + gpointer user_data; +}; + one space @@ +371,3 @@ + +/* work around a limitation of the aliasing foo */ +#undef g_win32_registry_key I do not understand this, why? @@ +378,3 @@ +g_win32_registry_key_dispose (GObject *object) +{ + GWin32RegistryKey *self; self -> key @@ +379,3 @@ +{ + GWin32RegistryKey *self; + GWin32RegistryKeyPrivate *priv; empty line between assign and declarations @@ +462,3 @@ +g_win32_registry_key_open_w (const gunichar2 *path, + GError **error) +{ in my opinion this method should just return g_initable_new(type, "path", path, NULL); @@ +481,3 @@ + + first_chunk_len = first_chunk_end - path; + first_chunk = g_memdup (path, (first_chunk_len + 1) * sizeof (gunichar2)); mmm is this really needed? why not just using wcsncmp? @@ +503,3 @@ + else + { + g_critical ("Root key '%S' is not a pre-defined key", first_chunk); this should set the passed in error @@ +512,3 @@ + second_chunk_begin = first_chunk_end; + + while (second_chunk_begin[0] != L'\0' && second_chunk_begin[0] == L'\\') mmm why not just calling wcschr here again? @@ +531,3 @@ + + result = g_object_new (G_TYPE_WIN32_REGISTRY_KEY, NULL); + result->priv->ancestor = NULL; keep a priv local member @@ +532,3 @@ + result = g_object_new (G_TYPE_WIN32_REGISTRY_KEY, NULL); + result->priv->ancestor = NULL; + result->priv->handle = key_handle; this should be a property? @@ +533,3 @@ + result->priv->ancestor = NULL; + result->priv->handle = key_handle; + result->priv->absolute_path_w = g_memdup (path, property? @@ +535,3 @@ + result->priv->absolute_path_w = g_memdup (path, + (wcslen (path) + 1) * sizeof (gunichar2)); + result->priv->predefined = (second_chunk_begin[0] == L'\0'); property? @@ +536,3 @@ + (wcslen (path) + 1) * sizeof (gunichar2)); + result->priv->predefined = (second_chunk_begin[0] == L'\0'); + result->priv->change_indicator = G_WIN32_KEY_UNKNOWN; I guess this should be set in the init method as the default value @@ +590,3 @@ + const gunichar2 *subkey, + GError **error) +{ this should probably do the same, g_initable_new(...); @@ +604,3 @@ + if (subkey[0] == L'\\') + { + g_critical ("Subkey name '%S' starts with '\\'", subkey); set the error @@ +616,3 @@ + if (end_of_subkey[0] == L'\\') + { + g_critical ("Subkey name '%S' ends with '\\'", subkey); set the error @@ +764,3 @@ + * data, without converting it to UTF-8 first. + * + * Returns: TRUE if next subkey info was retrieved, FALSE otherwise. %TRUE and %FALSE @@ +772,3 @@ + gunichar2 **subkey_name, + gsize *subkey_name_len, + gboolean skip_errors, I really don't like this skip_errors, teh calle can already skip the error by passing NULL @@ +784,3 @@ + if G_UNLIKELY (iter->counter >= iter->subkey_count) + { + g_critical ("g_win32_registry_subkey_iter_get_next_w: must not be called again " set the error? @@ +893,3 @@ + return FALSE; + + g_clear_pointer (&iter->subkey_name_u8, g_free); no need to clear_pointer, just use g_free since you are assigning it right after @@ +1096,3 @@ + gpointer *value_data, + gsize *value_data_size, + gboolean skip_errors, really I don't like this boolean @@ +1112,3 @@ + if G_UNLIKELY (iter->counter >= iter->value_count) + { + g_critical ("g_win32_registry_value_iter_get_next_w: must not be called " set the error @@ +1285,3 @@ + * + * while (g_win32_registry_value_iter_get_next (&iter, TRUE, &name, NULL, + * &val_type, &val_data, NULL, indentation? @@ +1298,3 @@ + * ]| + * + * Returns: TRUE if next value info was retrieved, FALSE otherwise. %TRUE, %FALSE @@ +1310,3 @@ + gpointer *value_data, + gsize *value_data_size, + gboolean skip_errors, kill it @@ +1331,3 @@ + return FALSE; + + g_clear_pointer (&iter->value_name_u8, g_free); just g_free @@ +1424,3 @@ + + /* Ensure 0-termination */ + ((char *) basic_info)[datasize] = 0; can we avoid left casting? @@ +1488,3 @@ + else + { + g_clear_pointer (&key->priv->absolute_path_w, g_free); just g_free @@ +1524,3 @@ + if (key->priv->absolute_path == NULL) + { + g_clear_pointer (&key->priv->absolute_path, g_free); just g_free @@ +1599,3 @@ + g_return_val_if_fail (error == NULL || *error == NULL, FALSE); + + /* No sense calling this function with all of these set to NULL */ is this something that should just return FALSE or is this really an assertion? @@ +1714,3 @@ + g_return_val_if_fail (error == NULL || *error == NULL, FALSE); + + /* No sense calling this functions with all of these set to NULL */ same as above @@ +1787,3 @@ + + /* Make sure stringy types are terminated */ + if (value_type_g == G_WIN32_REGISTRY_VALUE_EXPAND_STR || in general since I saw it in other ifs all around the code, add parenthesis to make it easier to understand @@ +1840,3 @@ + (gunichar2 *) req_value_data, value_name); + g_free (req_value_data); + g_clear_pointer (&value_data_expanded, g_free); just call g_free, since anyway we are returning ::: gio/gwin32registrykey.h @@ +16,3 @@ + * Public License along with this library; if not, see <http://www.gnu.org/licenses/>. + * + */ leave a newline here
(In reply to comment #16) > Review of attachment 285045 [details] [review]: > > See another round of reviews, it has some comments and questions inline. > > ::: gio/gwin32registrykey.c > @@ +371,3 @@ > + > +/* work around a limitation of the aliasing foo */ > +#undef g_win32_registry_key > > I do not understand this, why? > Dunno, copied from the source file i've used as a template. > @@ +378,3 @@ > +g_win32_registry_key_dispose (GObject *object) > +{ > + GWin32RegistryKey *self; > > self -> key > > @@ +379,3 @@ > +{ > + GWin32RegistryKey *self; > + GWin32RegistryKeyPrivate *priv; > > empty line between assign and declarations > > @@ +462,3 @@ > +g_win32_registry_key_open_w (const gunichar2 *path, > + GError **error) > +{ > > in my opinion this method should just return g_initable_new(type, "path", path, > NULL); Why? I've read on GInitable and looked at gsubprocess and gsocket, and i still don't quite understand why it does what it does. How do you imagine the use of GWin32RegistryKey will change, if it implements GInitable? > > @@ +481,3 @@ > + > + first_chunk_len = first_chunk_end - path; > + first_chunk = g_memdup (path, (first_chunk_len + 1) * sizeof (gunichar2)); > > mmm is this really needed? why not just using wcsncmp? wcsncmp would work, however: 1) I already have this code, and i'm too lazy to change it. 2) With wcsncmp error checking takes a slightly different path. Imagine if someone tries to open HKEY_LOCAL_MACHINEFOOO or HKEY_LOCAL_MACHINEBAR\\zool wcsncmp will correctly determine the ancestor key. Then it must check that the key name either ends in L'\0' (we open a pre-defined root key) or is followed by a L'\\'. If it does not, the function will return NULL with critical warning. I'm failing to come up with a way to throw that warning so that it becomes obvious that HKEY_LOCAL_MACHINEFOOO or HKEY_LOCAL_MACHINEBAR are not valid root key names. > > @@ +503,3 @@ > + else > + { > + g_critical ("Root key '%S' is not a pre-defined key", first_chunk); > > this should set the passed in error No. GError is for errors in *your* implementation or for passing errors from the *callees*. If the *caller* screws up (and giving a wrong *root* key name is really a screwup), it's a critical error. If what you're asking is for me to *also* set the error - *that* i can do (although it'll mean some kind of duplication - at the very least the error string will have to be specified twice). > @@ +512,3 @@ > + second_chunk_begin = first_chunk_end; > + > + while (second_chunk_begin[0] != L'\0' && second_chunk_begin[0] == L'\\') > > mmm why not just calling wcschr here again? This code starts looking after the root key name, then skips all L'\\' characters until it either hits L'\0' or runs out of L'\\'. wchchr will just find other L'\\' characters in the string, not constraining itself with the first uninterruptable run of L'\\' characters after the first chunk. Basically, it's for keys like this: HKEY_CURRENT_USER\\\\\\Software\\Gnome The "Software\\Gnome" is handled by Windows API, so i just let it deal with a possibility that multiple L'\\' in a row are used to separate key names in the path. But the root part and the separators after it are our responsibility. > > @@ +531,3 @@ > + > + result = g_object_new (G_TYPE_WIN32_REGISTRY_KEY, NULL); > + result->priv->ancestor = NULL; > > keep a priv local member What? > > @@ +532,3 @@ > + result = g_object_new (G_TYPE_WIN32_REGISTRY_KEY, NULL); > + result->priv->ancestor = NULL; > + result->priv->handle = key_handle; > > this should be a property? Possibly. Though i can't imagine why one would want to access this handle, given the APIs we already provide. > > @@ +533,3 @@ > + result->priv->ancestor = NULL; > + result->priv->handle = key_handle; > + result->priv->absolute_path_w = g_memdup (path, > > property? OK. > > @@ +535,3 @@ > + result->priv->absolute_path_w = g_memdup (path, > + (wcslen (path) + 1) * sizeof > (gunichar2)); > + result->priv->predefined = (second_chunk_begin[0] == L'\0'); > > property? OK. > @@ +590,3 @@ > + const gunichar2 *subkey, > + GError **error) > +{ > > this should probably do the same, g_initable_new(...); > See above. > @@ +604,3 @@ > + if (subkey[0] == L'\\') > + { > + g_critical ("Subkey name '%S' starts with '\\'", subkey); > > set the error See above. > > @@ +616,3 @@ > + if (end_of_subkey[0] == L'\\') > + { > + g_critical ("Subkey name '%S' ends with '\\'", subkey); > > set the error See above. > @@ +772,3 @@ > + gunichar2 > **subkey_name, > + gsize > *subkey_name_len, > + gboolean > skip_errors, > > I really don't like this skip_errors, teh calle can already skip the error by > passing NULL error == NULL means "don't report errors to me" skip_errors == TRUE means "don't fail if one of the subkeys/values can't be retrieved (for whatever reason)". This is for cases where the registry key changes while we're iterating over it. > > @@ +784,3 @@ > + if G_UNLIKELY (iter->counter >= iter->subkey_count) > + { > + g_critical ("g_win32_registry_subkey_iter_get_next_w: must not be called > again " > > set the error? See above > @@ +1096,3 @@ > + gpointer *value_data, > + gsize > *value_data_size, > + gboolean skip_errors, > > really I don't like this boolean See above. > > @@ +1112,3 @@ > + if G_UNLIKELY (iter->counter >= iter->value_count) > + { > + g_critical ("g_win32_registry_value_iter_get_next_w: must not be called > " > > set the error See above. The caller must not do this. > @@ +1310,3 @@ > + gpointer *value_data, > + gsize > *value_data_size, > + gboolean skip_errors, > > kill it See above. > @@ +1424,3 @@ > + > + /* Ensure 0-termination */ > + ((char *) basic_info)[datasize] = 0; > > can we avoid left casting? I can do this: char *basic_info_charptr = (char *) basic_info; ... basic_info_charptr[datasize] = 0; Would that be sufficient? > @@ +1599,3 @@ > + g_return_val_if_fail (error == NULL || *error == NULL, FALSE); > + > + /* No sense calling this function with all of these set to NULL */ > > is this something that should just return FALSE or is this really an assertion? It makes no sense to call this function like this. M-m-m...i guess you could use it to check that a value exists (regardless of its type and regardless of any data it may have). In that case such calls will be valid. However, it's best to also check the type of the variable (MS code always does), not just its existence. Unless there's a more compelling reason to change this, i'll keep this assertion.
Review of attachment 285045 [details] [review]: Just a couple of comments reading the h file (I have not looked at the implementation) ::: gio/gwin32registrykey.h @@ +90,3 @@ + * Since: 2.42 + */ +typedef void (*GWin32RegistryKeyWatchCallback) (GWin32RegistryKey *key, callback types are usually called with the "Func" suffix @@ +177,3 @@ +gsize g_win32_registry_subkey_iter_n_subkeys (GWin32RegistrySubkeyIter *iter); +GLIB_AVAILABLE_IN_2_42 +gboolean g_win32_registry_subkey_iter_get_next (GWin32RegistrySubkeyIter *iter, other iterator api in glib use _next() not _get_next() (g_hash_table_iter_next, g_sequence_iter_next, etc) I am also slightly surprised that get_next also gives the name as output, I'd have expected to just give me an iter and if I want the name just get it from the iter, however I am not familiar with the caller code, so maybe this is not practical if you always need the name.
(In reply to comment #18) > Review of attachment 285045 [details] [review]: > > I am also slightly surprised that get_next also gives the name as output, I'd > have expected to just give me an iter and if I want the name just get it from > the iter, however I am not familiar with the caller code, so maybe this is not > practical if you always need the name. This is definitely doable - make it return nothing but success/failure, and then add functions to get name from the iter.
Created attachment 285561 [details] [review] Add W32 Registry reading API to gio v2: changed all enumerators to iterators, changed tree watching, style fixes v3: fixed most of the nit v4: fixed character encoding in the header (d'oh!), fixed a typo v5: - g_win32_registry_key_watch{_for_changes,} - optional callback for g_win32_registry_key_watch - boxed types for iterators (no idea if they work; all i know is that the code compiles. - minor docs clarification - CHANGE -> WATCH - removed "Only <gio/gio.h> can be included directly" warning v6: - made GWin32RegistryKey a GInitable - added "path" and "path-utf16" properties, construction-only - g_wcsdup utility function (this should probably go into gstrfuncs.c eventually, but that's a whole different issue - comment fixes and annotation fixes - whitespace fixes - iters are allocated with g_new0 - more thorough checks for NULLs when dupping value iterators - iterators have a simple _next() method that only moves them, and a few new methods that get stuff (name, type, value) out of them. Expansion is now done in get_data[_w] (), there's a separate utf-8-encoded expanded data stored in the iterator (4 now - data, data_utf8, expanded_data and expanded_data_utf8). - GWin32RegistryKeyWatchCallback is now GWin32RegistryKeyWatchCallbackFunc - self->key - g_win32_registry_key_open -> g_win32_registry_key_new - g_win32_registry_key_open_subkey -> g_win32_registry_key_get_child - code deduplication (ensure_nul_termination() ensures NUL-termination, expand_value() expands G_WIN32_REGISTRY_VALUE_EXPAND_STR)
Review of attachment 285561 [details] [review]: I think we are almost there. let's get those nitpicks fixed and let's try to get ryan have it a look. ::: gio/gwin32registrykey.c @@ +124,3 @@ + +static gunichar2 * +g_wcsdup (const gunichar2 *str, gssize str_size) params in 2 lines @@ +511,3 @@ + g_return_val_if_fail (path != NULL, NULL); + + result = g_initable_new (G_TYPE_WIN32_REGISTRY_KEY, simply do return g_initable_new... @@ +803,3 @@ + g_return_if_fail (iter != NULL); + + g_clear_pointer (&iter->subkey_name, g_free); simply do g_free @@ +804,3 @@ + + g_clear_pointer (&iter->subkey_name, g_free); + g_clear_pointer (&iter->subkey_name_u8, g_free); same here @@ +1110,3 @@ + g_clear_pointer (&iter->value_data_u8, g_free); + g_clear_pointer (&iter->value_data_expanded, g_free); + g_clear_pointer (&iter->value_data_expanded_u8, g_free); for all these just use g_free @@ +1312,3 @@ + } + else if (status != ERROR_SUCCESS && skip_errors) + { no need for {} @@ +1770,3 @@ + else + { + g_clear_pointer (&key->priv->absolute_path_w, g_free); just g_free @@ +1807,3 @@ + if (key->priv->absolute_path == NULL) + { + g_clear_pointer (&key->priv->absolute_path, g_free); just g_free @@ +2406,3 @@ + g_object_class_install_property (gobject_class, + PROP_PATH_UTF16, + g_param_spec_pointer ("path-utf16", not sure about this name... ::: gio/gwin32registrykey.h @@ +259,3 @@ + +GLIB_AVAILABLE_IN_2_42 +const gchar * g_win32_registry_key_get_path (GWin32RegistryKey *key); attach the * to the method
Created attachment 285629 [details] [review] Add W32 Registry reading API to gio v2: changed all enumerators to iterators, changed tree watching, style fixes v3: fixed most of the nit v4: fixed character encoding in the header (d'oh!), fixed a typo v5: - g_win32_registry_key_watch{_for_changes,} - optional callback for g_win32_registry_key_watch - boxed types for iterators (no idea if they work; all i know is that the code compiles. - minor docs clarification - CHANGE -> WATCH - removed "Only <gio/gio.h> can be included directly" warning v6: - made GWin32RegistryKey a GInitable - added "path" and "path-utf16" properties, construction-only - g_wcsdup utility function (this should probably go into gstrfuncs.c eventually, but that's a whole different issue - comment fixes and annotation fixes - whitespace fixes - iters are allocated with g_new0 - more thorough checks for NULLs when dupping value iterators - iterators have a simple _next() method that only moves them, and a few new methods that get stuff (name, type, value) out of them. Expansion is now done in get_data[_w] (), there's a separate utf-8-encoded expanded data stored in the iterator (4 now - data, data_utf8, expanded_data and expanded_data_utf8). - GWin32RegistryKeyWatchCallback is now GWin32RegistryKeyWatchCallbackFunc - self->key - g_win32_registry_key_open -> g_win32_registry_key_new - g_win32_registry_key_open_subkey -> g_win32_registry_key_get_child - code deduplication (ensure_nul_termination() ensures NUL-termination, expand_value() expands G_WIN32_REGISTRY_VALUE_EXPAND_STR) v7: - use g_free() instead of g_clear_pointer(), where appropriate - indentation and whitespace fixes
(In reply to comment #21) > Review of attachment 285561 [details] [review]: > > @@ +2406,3 @@ > + g_object_class_install_property (gobject_class, > + PROP_PATH_UTF16, > + g_param_spec_pointer ("path-utf16", > > not sure about this name... What other options do i have? "path-utf16" seems a logical choice. As for the discussion that i've missed on the IRC, i'm definitely OK with keeping this API internal (or, at least, not giving end-users any promises that this API won't change, but still exposing it). Rewriting GSettings registry backend with it will probably require expanding the API to make it capable of creating new keys, deleting existing ones, maybe renaming them too, and setting/removing values. I have not given this expansion much thought, so i'm not sure, at the moment, how to best design it. Also, i don't have code (other than GSettings registry backend, with which i'm not really familiar) to test the would-be-expanded API.
I'd say fix the minor issues pointed out, and make it internal api for now, this way we can continue with the other issues you want to reach. This way we can give this api more testing and in the future also port the gsettings backend to it.
Created attachment 285633 [details] [review] Add W32 Registry reading API to gio v2: changed all enumerators to iterators, changed tree watching, style fixes v3: fixed most of the nit v4: fixed character encoding in the header (d'oh!), fixed a typo v5: - g_win32_registry_key_watch{_for_changes,} - optional callback for g_win32_registry_key_watch - boxed types for iterators (no idea if they work; all i know is that the code compiles. - minor docs clarification - CHANGE -> WATCH - removed "Only <gio/gio.h> can be included directly" warning v6: - made GWin32RegistryKey a GInitable - added "path" and "path-utf16" properties, construction-only - g_wcsdup utility function (this should probably go into gstrfuncs.c eventually, but that's a whole different issue - comment fixes and annotation fixes - whitespace fixes - iters are allocated with g_new0 - more thorough checks for NULLs when dupping value iterators - iterators have a simple _next() method that only moves them, and a few new methods that get stuff (name, type, value) out of them. Expansion is now done in get_data[_w] (), there's a separate utf-8-encoded expanded data stored in the iterator (4 now - data, data_utf8, expanded_data and expanded_data_utf8). - GWin32RegistryKeyWatchCallback is now GWin32RegistryKeyWatchCallbackFunc - self->key - g_win32_registry_key_open -> g_win32_registry_key_new - g_win32_registry_key_open_subkey -> g_win32_registry_key_get_child - code deduplication (ensure_nul_termination() ensures NUL-termination, expand_value() expands G_WIN32_REGISTRY_VALUE_EXPAND_STR) v7: - use g_free() instead of g_clear_pointer(), where appropriate - indentation and whitespace fixes v8: - don't install the header (make the API internal)
Review of attachment 285633 [details] [review]: Hi LRN, Few issues about this patch: There are some trailing whitespaces here, so please check those out. ::: gio/gwin32registrykey.c @@ +47,3 @@ +#endif + +#ifndef __OBJECT_ATTRIBUTES_DEFINED _OBJECT_ATTRIBUTES and _UNICODE_STRING are included in the Visual Studio's winternl.h, so we need to define __OBJECT_ATTRIBUTES_DEFINED and __UNICODE_STRING_DEFINED when we have _MSC_VER defined, otherwise the build breaks. @@ +69,3 @@ +#endif + +#ifndef __UNICODE_STRING_DEFINED Please see above comment. We also need to use #pragma warning ( disable:4005 ) before including the Windows headers, when we have _MSC_VER defined, as the status codes are both included from winnt.h and ntstatus.h, to avoid macro redefinition warnings. @@ +80,3 @@ + +typedef NTSTATUS NTAPI +(* NtQueryKeyFunc)(HANDLE key_handle, This breaks the build on Visual Studio, it expects something like: typedef NTSTATUS (NTAPI * NtQueryKeyFunc)... It is picky about where the __stdcall (which is what NTAPI is) is placed, up to even the latest MSVC version... @@ +145,3 @@ + * free with g_win32_registry_subkey_iter_free () + * + * Since: 2.42 Let's do Since: 2.46... @@ +182,3 @@ +g_win32_registry_subkey_iter_free (GWin32RegistrySubkeyIter *iter) +{ + g_return_val_if_fail (iter != NULL, NULL); Please use g_return_if_fail() @@ +286,3 @@ +g_win32_registry_value_iter_free (GWin32RegistryValueIter *iter) +{ + g_return_val_if_fail (iter != NULL, NULL); Please use g_return_if_fail() @@ +543,3 @@ + + g_return_val_if_fail (G_IS_WIN32_REGISTRY_KEY (initable), NULL); + g_return_val_if_fail (error == NULL || *error == NULL, NULL); For the above 2 lines, we need g_return_val_if_fail (..., FALSE), not NULL. MSVC builds don't like NULL here. @@ +1742,3 @@ + */ + if (nt_query_key != NULL && !key->priv->predefined) + return _g_win32_registry_key_reread_kernel (key, buf); Please omit "return" @@ +1744,3 @@ + return _g_win32_registry_key_reread_kernel (key, buf); + else + return _g_win32_registry_key_reread_user (key, buf); Same here. @@ +2247,3 @@ + + g_atomic_int_set (&key->priv->change_indicator, G_WIN32_KEY_UNKNOWN); + g_atomic_int_set (&key->priv->watch_indicator, G_WIN32_KEY_UNWATCHED); Somehow I am getting an access violation here when doing the g_atomic_int_set()...
(In reply to Fan, Chun-wei from comment #26) > Review of attachment 285633 [details] [review] [review]: > > Hi LRN, > > Few issues about this patch: You'll have to fix some of these yourself, as anything i do is pointless, since i have no MSVC to test any of this. I can only introduce simple, obvious changes and maybe redesign parts of code in a way that will make it easier for you to patch it up for MSVC compatibility, but that's it. > ::: gio/gwin32registrykey.c > @@ +47,3 @@ > +#endif > + > +#ifndef __OBJECT_ATTRIBUTES_DEFINED > > _OBJECT_ATTRIBUTES and _UNICODE_STRING are included in the Visual Studio's > winternl.h, so we need to define __OBJECT_ATTRIBUTES_DEFINED and > __UNICODE_STRING_DEFINED when we have _MSC_VER defined, otherwise the build > breaks. > > @@ +69,3 @@ > +#endif > + > +#ifndef __UNICODE_STRING_DEFINED > > Please see above comment. Does that mean that _OBJECT_ATTRIBUTES and _UNICODE_STRING just come with the headers normally (no macros needed to get them) and that MS SDK does not define things like __UNICODE_STRING_DEFINED? I'm a bit lost here. > > We also need to use #pragma warning ( disable:4005 ) before including the > Windows headers, when we have _MSC_VER defined, as the status codes are both > included from winnt.h and ntstatus.h, to avoid macro redefinition warnings. > This one will have to be patched by you. > > @@ +145,3 @@ > + * free with g_win32_registry_subkey_iter_free () > + * > + * Since: 2.42 > > Let's do Since: 2.46... Right. I'm actually not sure how the "Since" thing is handled (is it synced manually with the version at which the patch actually lands? Or is there some kind of tool to do that?). > > @@ +2247,3 @@ > + > + g_atomic_int_set (&key->priv->change_indicator, G_WIN32_KEY_UNKNOWN); > + g_atomic_int_set (&key->priv->watch_indicator, G_WIN32_KEY_UNWATCHED); > > Somehow I am getting an access violation here when doing the > g_atomic_int_set()... It could be that the function that g_atomic_int_set() actually calls expects different variable size or something like that. No idea. The code from gatomic.c seems to be (for MSVC): *atomic = newval; MemoryBarrier ();
Hello LRN, (In reply to LRN from comment #27) > > You'll have to fix some of these yourself, as anything i do is pointless, > since i have no MSVC to test any of this. I can only introduce simple, > obvious changes and maybe redesign parts of code in a way that will make it > easier for you to patch it up for MSVC compatibility, but that's it. OK, I see. I could update your patch though for the really MSVC-specific cases. > > > ::: gio/gwin32registrykey.c > Does that mean that _OBJECT_ATTRIBUTES and _UNICODE_STRING just come with > the headers normally (no macros needed to get them) and that MS SDK does not > define things like __UNICODE_STRING_DEFINED? I'm a bit lost here. That is correct. > > > > > We also need to use #pragma warning ( disable:4005 ) before including the > > Windows headers, when we have _MSC_VER defined, as the status codes are both > > included from winnt.h and ntstatus.h, to avoid macro redefinition warnings. > > > > This one will have to be patched by you. Right. > > > > > @@ +145,3 @@ > > + * free with g_win32_registry_subkey_iter_free () > > + * > > + * Since: 2.42 > > > > Let's do Since: 2.46... > > Right. I'm actually not sure how the "Since" thing is handled (is it synced > manually with the version at which the patch actually lands? Or is there > some kind of tool to do that?). Nope, done manually. It means the first stable series that this API is added. > > It could be that the function that g_atomic_int_set() actually calls expects > different variable size or something like that. No idea. > > The code from gatomic.c seems to be (for MSVC): > > *atomic = newval; > MemoryBarrier (); Yup, this is what I suspected... A bad pointer/address for *atomic... Will check further.
Created attachment 302906 [details] [review] Make W32 registry API compatible with MSVC * Only check __OBJECT_ATTRIBUTES_DEFINED and __UNICODE_STRING_DEFINED on MinGW (MSVC doesn't have these) * MSVC: disable:4005 when including windows.h and ntstatus.h * Move NTAPI cconv into the parens with the NtQueryKeyFunc * Fix return values in some functions
Created attachment 302907 [details] [review] Bump W32 Registry API 'Since:' version