Win32 port

[Peter Kümmel]

Havoc Pennington wrote:
> Hi,
>
> Cool. Where is the string coming from on Windows? (Am I just missing
> that part of the patch?) What I'm wondering is how does it get
> allocated/freed.
>

I have not added any win32 code, only changed the unix interfaces.
Below you could see the function which returns the string, but it
is the old code not using DBusUID.

Index: dbus/dbus-sysdeps.c
===================================================================
RCS file: /cvs/dbus/dbus/dbus/dbus-sysdeps.c,v
retrieving revision 1.102
diff -u -b -B -r1.102 dbus-sysdeps.c
--- dbus/dbus-sysdeps.c    30 May 2006 15:34:10 -0000    1.102
+++ dbus/dbus-sysdeps.c    22 Jun 2006 11:39:20 -0000

+/**
+ * Returns the textual form of a SID. The return value points
+ * to statically allocated memory and should not be freed.
+ *
+ * @param psid pointer to the SID
+ */
+dbus_uid_t
+_dbus_win32_sid_to_uid_t (void *psid)
+{
+  dbus_uid_t retval;
+  char *string, *oldval;
+
+  if (!IsValidSid ((PSID) psid)) {
+   _dbus_verbose("%s invalid sid\n",__FUNCTION__);
+    return "-";
+    }
+  if (!ConvertSidToStringSidA ((PSID) psid, &string)) {
+    _dbus_verbose("%s invalid sid\n",__FUNCTION__);
+    return "-";
+  }
+
+  _DBUS_LOCK (win32_sids);
+
+  if (win32_sids == NULL)
+    {
+      win32_sids = _dbus_hash_table_new (DBUS_HASH_STRING, NULL, NULL);
+      _dbus_register_shutdown_func (sid_cache_shutdown, NULL);
+    }
+
+  oldval = _dbus_hash_table_lookup_string (win32_sids, string);
+
+  if (oldval)
+    {
+      _dbus_verbose("%s sid %s found in cache\n",__FUNCTION__,oldval);
+      LocalFree (string);
+      retval = oldval;
+    }
+  else
+    {
+      _dbus_hash_table_insert_string (win32_sids, string, string);
+      _dbus_verbose("%s sid %s added to cache\n",__FUNCTION__,string);
+      retval = string;
+    }
+
+  _DBUS_UNLOCK (win32_sids);
+
+  return retval;
+}
+
+#endif
+
 /** @} end of sysdeps */

 /* tests in dbus-sysdeps-util.c */

 /** @} end of sysdeps */

 /* tests in dbus-sysdeps-util.c */



Seems Tor already has implemented a memory management:
the strings are cached, and freed at shutdown.
So we could your approach 2. "true value objects", or is
it more the 3. way?

I'm not 100% sure how I should proceed. I've here a
200k diff against HEAD and some new files.

Should I just post the patches related to the changes to
the original types?

Does it help when I post the diff and new files?


> There's a pretty good test suite in dbus so don't be afraid to rip 'er
> up and refactor, in most cases breakage will be revealed by the suite.
> 
> There are maybe four kinds of opaque object in common use in
> dbus/glib/gtk style C programming...
> 
>  1. "allocated value objects"
>     Can be identified by the presence of _new(), _free(), optional
>     _copy() methods, passed around as a pointer (e.g. DBusMemPool,
>     DBusMutex, GHashTable, GError, GMarkupParseContext)
> 
>  2. "true value objects"
>     Can be copied with C assignment operator and allocated on stack;
>     may have an _init() method but not _free() (e.g. DBusMessageIter,
>     DBusSHAContext, GtkTextIter, GdkColor)
> 
>  3. "weird hybrid of 1 and 2"
>     The usage is to allocate on stack, _init(), then later _free();
>     using C assignment operator isn't allowed (e.g. DBusString,
>     DBusError, DBusUserInfo). I use this a lot in dbus, but it's
>     not used much in glib/gtk since #1 can always be used instead.
>     The main value of the "weird hybrid" is that you avoid a memory
>     allocation, which in most cases is silly overoptimization.
> 
>  4. "refcounted object"
>     Similar to #1, but the object is reference counted rather than
>     having a flat free() method.[1] (e.g. GObject, DBusConnection)
> 
> I'd rather DBusUID fit into one of these patterns. If there's no memory
> management on that windows UID, then you could use pattern #2, which
> would look something like:
> 
> .h:
> 
> /* it's OK to copy this by value; it will never have allocated fields */
> typedef struct {
>   void *dummy;
> } DBusUID;
> 
> .c:
> 
> typedef struct {
>   union {
>     unsigned long unix;
>     char *windows;
>   } uid;
> } DBusUIDReal;
> 
> void
> _dbus_uid_init_unix(DBusUID *uid, unsigned long value)
> {
>   DBusUIDReal *real = (DBusUIDReal*) uid;
>   _dbus_assert(sizeof(DBusUIDReal) <= sizeof(DBusUID));
>   real->uid.unix = value;
> }
> 
> That would involve relatively few changes to code, though I'd still
> rather pass it in to functions as "DBusUID *uid" and not "DBusUID uid"
> to match the usual pattern.

I don't see the advantage of this approach, because we have to change
much code to introduce DBusUID so it does not care how we change it.
We could also pass pointers using my approach.

And isn't it better to have a static type checking instead of the
dynamic sizeof check?

> If the Windows UID needs memory management though, you need to use
> approach 1 or 3, approach 2 simply won't work. Either 1 or 3 is fine,
> they are more or less equivalent and should be the same amount of work.
> With 1 and 3 passing by pointer and not value is of course required
> since C assignment isn't allowed.
> 
> btw, what about guid_t? What's up with that on Windows, should it get a
> parallel/identical treatment?
> 

I first wanna see what happens when DBusUID is introduced and how the
merge process will work.

> Havoc
> 
> [1] Sub-rule for refcounted objects if you're interested; for language
> binding reasons, public API objects have to be either immutable (so they
> can be treated as a by-value object despite the refcount) or they have
> to have "destroy notification" (so the language binding proxy can be
> deleted on finalization)
> 
>