|
Description
Ole André Vadla Ravnås
2010-03-22 08:41:37 UTC
Created attachment 34313 [details] [review] Add a wrapper for realpath to support non-POSIX OSes Created attachment 34314 [details] [review] Fix a trivial warning where guint was used instead of WockyTLSCertStatus Created attachment 34315 [details] [review] Add a missing cast to make MSVC a bit happier warning-wise Created attachment 34316 [details] [review] Fix warnings related to DEBUG() usage without arguments Created attachment 34317 [details] [review] Make MUC timestamp parsing portable Could need some careful review. Created attachment 34318 [details] [review] Fix OpenSSL include issue on Windows Created attachment 34319 [details] [review] Replace CA/CRL stat logic with g_file_test to improve portability Created attachment 34320 [details] [review] Avoid C99 named initializers Created attachment 34321 [details] [review] Replace isXXX() with g_ascii_isXXX() to improve portability Created attachment 34322 [details] [review] Fix error_to_string() crash That's all so far. Am now able to connect to Google Talk from my Vala/GTK+ test application. Regarding the first patch: configure.ac might need to be updated, not certain if any of the existing M4 macros check for unistd.h. (Haven't had time to test this on a UNIX system yet.) Do you have a git repo/branch online somewhere for this sort of thing? We can resurrect your git.collabora.co.uk account if that'd help? Comment on attachment 34312 [details] [review] Don't include unistd.h unless available >Subject: [PATCH 01/12] Don't include unistd.h unless available Looks good in principle. We don't normally wrap config.h in HAVE_CONFIG_H: is creating a stub config.h problematic on Windows? > #include <stdio.h> >+#ifdef HAVE_UNISTD_H > #include <unistd.h> >+#endif > #include <stdlib.h> > #include <string.h> I might add a patch to do the unconditional and conditional includes separately, and add a bit of whitespace: #include <stdio.h> #include <stdlib.h> #include <string.h> #ifdef HAVE_HYPOTHETICAL_H # include <hypothetical.h> #endif #ifdef HAVE_UNISTD_H # include <unistd.h> #endif Comment on attachment 34313 [details] [review] Add a wrapper for realpath to support non-POSIX OSes >Subject: [PATCH 02/12] Add a wrapper for realpath to support non-POSIX OSes >+gchar * >+wocky_normalise_path (const gchar *path) >+{ >+#ifdef G_OS_WIN32 >+ if (!g_path_is_absolute (path)) >+ return NULL; >+ return g_strdup (path); >+#else >+ return realpath (path, NULL); >+#endif >+} I'm pretty sure that's wrong for relative paths. Wouldn't we be better off using g_file_resolve_relative_path (g_file_new_for_path (g_get_current_dir ()), path) on Windows? (If we don't really care about normalizing symlinks, we could probably even do the same on Unix.) Comment on attachment 34314 [details] [review] Fix a trivial warning where guint was used instead of WockyTLSCertStatus >Subject: [PATCH 03/12] Fix a trivial warning where guint was used instead of WockyTLSCertStatus Looks good Comment on attachment 34315 [details] [review] Add a missing cast to make MSVC a bit happier warning-wise >Subject: [PATCH 04/12] Add a missing cast to make MSVC a bit happier warning-wise I approve Comment on attachment 34316 [details] [review] Fix warnings related to DEBUG() usage without arguments >Subject: [PATCH 05/12] Fix warnings related to DEBUG() usage without arguments Looks like a necessary evil, and would be nice to put on the /Portability page too Comment on attachment 34317 [details] [review] Make MUC timestamp parsing portable >Subject: [PATCH 06/12] Make MUC timestamp parsing portable I made a very similar change in Gabble a while ago, which never got merged... >+ GTimeVal when; >+ >+ if (g_time_val_from_iso8601 (tm, &when)) > DEBUG ("Malformed date string '%s' for " WOCKY_XMPP_NS_DELAY, tm); > else >- stamp = timegm (&when); >+ stamp = when.tv_sec; ... but I think you're missing a "!" at the beginning of the if condition :-) Comment on attachment 34318 [details] [review] Fix OpenSSL include issue on Windows >Subject: [PATCH 07/12] Fix OpenSSL include issue on Windows > >OpenSSL's Windows headers apparently require windows.h to be included >before including them. Yikes. Seems to be a necessary evil... Comment on attachment 34319 [details] [review] Replace CA/CRL stat logic with g_file_test to improve portability >Subject: [PATCH 08/12] Replace CA/CRL stat logic with g_file_test to improve portability Looks good to me Comment on attachment 34320 [details] [review] Avoid C99 named initializers >Subject: [PATCH 09/12] Avoid C99 named initializers wjt will be sad, but it seems necessary. In existing code we've generally used a style more like: foo, /* foo */ NULL, /* bar */ > static xmlSAXHandler parser_handler = { >- .initialized = XML_SAX2_MAGIC, >- .startElementNs = _start_element_ns, >- .endElementNs = _end_element_ns, >- .characters = _characters, >- .serror = _error, >+ /* internalSubset */ NULL, >+ /* isStandalone */ NULL, >+ /* hasInternalSubset */ NULL, >+ /* hasExternalSubset */ NULL, >+ /* resolveEntity */ NULL, >+ /* getEntity */ NULL, >+ /* entityDecl */ NULL, >+ /* notationDecl */ NULL, >+ /* attributeDecl */ NULL, >+ /* elementDecl */ NULL, >+ /* unparsedEntityDecl */ NULL, >+ /* setDocumentLocator */ NULL, >+ /* startDocument */ NULL, >+ /* endDocument */ NULL, >+ /* startElement */ NULL, >+ /* endElement */ NULL, >+ /* reference */ NULL, >+ /* characters */ _characters, >+ /* ignorableWhitespace */ NULL, >+ /* processingInstruction */ NULL, >+ /* comment */ NULL, >+ /* warning */ NULL, >+ /* error */ NULL, >+ /* fatalError */ NULL, >+ /* getParameterEntity */ NULL, >+ /* cdataBlock */ NULL, >+ /* externalSubset */ NULL, >+ /* initialized */ XML_SAX2_MAGIC, >+ /* _private */ NULL, >+ /* startElementNs */ _start_element_ns, >+ /* endElementNs */ _end_element_ns, >+ /* serror */ _error > }; I haven't yet verified that these are in the right order. Comment on attachment 34321 [details] [review] Replace isXXX() with g_ascii_isXXX() to improve portability >Subject: [PATCH 10/12] Replace isXXX() with g_ascii_isXXX() to improve portability Yes please! Comment on attachment 34322 [details] [review] Fix error_to_string() crash >Subject: [PATCH 11/12] Fix error_to_string() crash > >Declaring the static ssl_error array as const means that it may be read- >only at runtime, which is not what you want in this case. Yes please! I don't know why anyone thought this was right... also, the (gchar *) cast ~10 lines later is a symptom of this mistake (which can now be removed), and the (const gchar *) cast the line after that is unnecessary. (In reply to comment #11) > not certain > if any of the existing M4 macros check for unistd.h I'll herd the patches I approved into a branch, and make sure that check is there, unless someone beats me to it. http://git.collabora.co.uk/?p=user/smcv/wocky.git;a=shortlog;h=refs/heads/oleavr-windows contains all those patches except for the realpath wrapper and MUC timestamp parsing, plus some minor changes from me. (In reply to comment #18) > (From update of attachment 34317 [details] [review]) > >Subject: [PATCH 06/12] Make MUC timestamp parsing portable > > I made a very similar change in Gabble a while ago which is this: http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=7c0dac9e3fb52cb1ef The timestamps seen in MUC do not contain an explicit timezone, but according to the XEP, they are understood to be in UTC, therefore we need to append "Z" before calling g_time_val_to_iso8601. (In reply to comment #12) > Do you have a git repo/branch online somewhere for this sort of thing? We can > resurrect your git.collabora.co.uk account if that'd help? I do have a fdo account, but I didn't bother pushing there as I got pointed to this bugzilla, so I assumed I should attach patches here as well. :) Should I push there, or what's the easiest for you? (In reply to comment #13) > (From update of attachment 34312 [details] [review]) > >Subject: [PATCH 01/12] Don't include unistd.h unless available > > Looks good in principle. We don't normally wrap config.h in HAVE_CONFIG_H: is > creating a stub config.h problematic on Windows? Ah good. That was a broken assumption on my side -- hsbuild (which I'm using on Windows), auto-generates a config.h, so that's fine. > > #include <stdio.h> > >+#ifdef HAVE_UNISTD_H > > #include <unistd.h> > >+#endif > > #include <stdlib.h> > > #include <string.h> > > I might add a patch to do the unconditional and conditional includes > separately, and add a bit of whitespace: > > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > > #ifdef HAVE_HYPOTHETICAL_H > # include <hypothetical.h> > #endif > #ifdef HAVE_UNISTD_H > # include <unistd.h> > #endif Yup, that definitely looks better! (In reply to comment #14) > (From update of attachment 34313 [details] [review]) > >Subject: [PATCH 02/12] Add a wrapper for realpath to support non-POSIX OSes > > >+gchar * > >+wocky_normalise_path (const gchar *path) > >+{ > >+#ifdef G_OS_WIN32 > >+ if (!g_path_is_absolute (path)) > >+ return NULL; > >+ return g_strdup (path); > >+#else > >+ return realpath (path, NULL); > >+#endif > >+} > > I'm pretty sure that's wrong for relative paths. Wouldn't we be better off > using g_file_resolve_relative_path (g_file_new_for_path (g_get_current_dir ()), > path) on Windows? > > (If we don't really care about normalizing symlinks, we could probably even do > the same on Unix.) Yes, definitely, that'd be great! I meant to include a comment that this one could need a real implementation on Windows, the whole thing was a bit rushed. :) (In reply to comment #18) > (From update of attachment 34317 [details] [review]) > >Subject: [PATCH 06/12] Make MUC timestamp parsing portable > > I made a very similar change in Gabble a while ago, which never got merged... > > >+ GTimeVal when; > >+ > >+ if (g_time_val_from_iso8601 (tm, &when)) > > DEBUG ("Malformed date string '%s' for " WOCKY_XMPP_NS_DELAY, tm); > > else > >- stamp = timegm (&when); > >+ stamp = when.tv_sec; > > ... but I think you're missing a "!" at the beginning of the if condition :-) > Oops! -ENOCAFFEINE :) (In reply to comment #26) > (In reply to comment #18) > > (From update of attachment 34317 [details] [review] [details]) > > >Subject: [PATCH 06/12] Make MUC timestamp parsing portable > > > > I made a very similar change in Gabble a while ago > > which is this: > > http://git.collabora.co.uk/?p=user/smcv/telepathy-gabble-smcv.git;a=commitdiff;h=7c0dac9e3fb52cb1ef > > The timestamps seen in MUC do not contain an explicit timezone, but according > to the XEP, they are understood to be in UTC, therefore we need to append "Z" > before calling g_time_val_to_iso8601. > Great stuff! Thanks a lot! (In reply to comment #25) > http://git.collabora.co.uk/?p=user/smcv/wocky.git;a=shortlog;h=refs/heads/oleavr-windows > contains all those patches except for the realpath wrapper and MUC timestamp > parsing, plus some minor changes from me. > Sweet! Thanks! I'll pull and look into the remaining bits. One more thing, slightly off-topic, would you be okay with changing wocky_connector_{connect,register}_finish so the GError is put as the last argument instead of middle? That would make Vala async binding "just work". :-) (In reply to comment #14) > (From update of attachment 34313 [details] [review]) > >Subject: [PATCH 02/12] Add a wrapper for realpath to support non-POSIX OSes I now realise that the previous use of realpath() was wrong: we g_free its result, but g_free is not guaranteed to be compatible with free(). > I'm pretty sure that's wrong for relative paths. Wouldn't we be better off > using g_file_resolve_relative_path (g_file_new_for_path (g_get_current_dir ()), > path) on Windows? > > (If we don't really care about normalizing symlinks, we could probably even do > the same on Unix.) The two uses of this are to load CRLs and CA certificates, neither of which seems to me to really need this functionality? I've added a patch to the branch which implements the rest, using GFile. (In reply to comment #32) > One more thing, slightly off-topic, would you be okay with changing > wocky_connector_{connect,register}_finish so the GError is put as the last > argument instead of middle? That would make Vala async binding "just work". :-) I for one would be in favour of this change. (In reply to comment #33) > (In reply to comment #14) > > (From update of attachment 34313 [details] [review] [details]) > > >Subject: [PATCH 02/12] Add a wrapper for realpath to support non-POSIX OSes > > I now realise that the previous use of realpath() was wrong: we g_free its > result, but g_free is not guaranteed to be compatible with free(). Sorry, I remember going "what?!" when I read that bit, but forgot entirely about it. Good thing you noticed it! > > I'm pretty sure that's wrong for relative paths. Wouldn't we be better off > > using g_file_resolve_relative_path (g_file_new_for_path (g_get_current_dir ()), > > path) on Windows? > > > > (If we don't really care about normalizing symlinks, we could probably even do > > the same on Unix.) > > The two uses of this are to load CRLs and CA certificates, neither of which > seems to me to really need this functionality? I've added a patch to the branch > which implements the rest, using GFile. Rock! \m/ Ole André, Sjoerd: care to review <http://git.collabora.co.uk/?p=user/smcv/wocky.git;a=shortlog;h=refs/heads/oleavr-windows>? I think it's ready. (In reply to comment #36) > Ole André, Sjoerd: care to review > <http://git.collabora.co.uk/?p=user/smcv/wocky.git;a=shortlog;h=refs/heads/oleavr-windows>? > I think it's ready. Looks great! Fixed in git, thanks! |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.