UUID functions missing on Windows

[Charlie Hull]

There is no equivalent to uuid/uuid.h on Windows, and some naming conflicts.

[Charlie Hull]

Patch to add uuid functions

[Richard Boulton]

It would probably be better if the patch also added a "common/safeuuid.h" file, which included the uuid/uuid.h or common/msvc_uuid.h, depending on the platform, to avoid having windows specific code scattered through several files. You could model this on "common/safedirent.h".

Is rpcrt4.lib a system DLL needed for the msvc_uuid stuff on windows? Or is it related to something else?

There seem to be several changes to things in xapian-maintainer-tools which are unrelated to this patch (dropping quartz and adding chert, for example) - I'll ignore these for now. (If you send me a patch with just those updates, I'll apply it separately, but it's best to keep patches single-function, for cleanness.)

[Olly Betts]

/* msvc_uuid.cc: Provides UUID functions with Posix semantics

I don't think that's correct - AFAIK, POSIX hasn't standardised an API for this, and the UUID formats were standardised by OSF if wikipedia is to be believed.

Also, it's "POSIX", not "Posix".

I'd just says "compatible with libuuid from e2fsprogs".

[Olly Betts]

Some other comments:

• I think msvc_uuid.{h,cc} is the wrong name choice - it's not specific to the compiler MSVC, but to the platform. I suggest win32_uuid.{h,cc} since "win32" is arguably the relevant aspect of the platform (and this is a wrapper around the win32 UUID API functions).

• I think msvc_uuid.h should error out if __WIN32__ isn't defined - there's no reason to include it on other platforms, so it's better to catch if we do (c.f. safewindows.h and safewinsock2.h).

• One of the files lacks a '\n' at the end, which some compilers don't handle (I'll add a note about that to HACKING).

• The code formatting doesn't follow the rest of Xapian in several regards (especially noticeable is the indentation of "}" and spacing around "(" and ")").

[Olly Betts]

OK, the bug in the patch is that you're taking the address of uuid_t in various places.

But uuid_t is an array type, so decays to a pointer, so e.g. memset(&uu, 0, 16) blats a pointer to uu (plus another 12 random bytes) rather than zeroing uu itself...

I also notice the function prototypes don't actually match those in uuid.h (some "const" missing).

[Olly Betts]

1.0.x doesn't have database UUIDs, so setting a saner milestone!

[Richard Boulton]

Charlie mentioned that he hasn't had time to look at this week, and that he'll take a look again next week.

[Charlie Hull]

Here's an improved patch. I've done my best to fit with existing Xapian code.

[Charlie Hull]

Better patch adding UUID functions

[Olly Betts]

Just to clarify, with this patch do the tests all pass (removing any cached databases in .flint and .chert first)?

(I'm sure someone said the previous patch didn't work, though that must have been in a different bug or by email as I don't see it noted here, other than implicitly by me pointing to the problem...)

[Olly Betts]

The ChangeLog's are messed up oddly. Looks like your xapian-core ChangeLog diff is against a copy of the msvc makefiles' ChangeLog with conflicts in, and the new ChangeLog entry for xapian-core was added to the the msvc makefiles' ChangeLog. But don't worry about that now - just bear it in mind for the future.

One issue I can see - uuid_generate() isn't allowed to fail, but your uuid_generate() won't generate a uuid if UuidCreate() doesn't return RPC_S_OK (libuuid generates a time-based UUID if it can't generate a network-address-based one). I think the main case here is a machine without a network card though the MSDN docs are a bit confusing.

It sounds like Vista must always generates time based UUIDs, and it hints that anything newer than 98 falls back to doing so without a network card, but then why is RPC_S_UUID_NO_ADDRESS a possible return code?

The current handling is particularly bad as it will quietly use whatever happens to be in the buffer already. If we can't sanely handle this, throwing DatabaseCreateError() would be better I think, though that might prevent creating a database at all on non-network systems...

[Charlie Hull]

How about

void uuid_generate(uuid_t uu)
{
    UUID uuid;
    memset(&uuid,0x00,16);
    UuidCreate(&uuid);
    memcpy(uu,&uuid,16);
}

To answer an earlier comment, no, the tests don't all pass yet, but at least the code compiles! I'll try and look at test failures soon.

[Olly Betts]

But then different databases can have the same UUID, which makes the feature rather less useful.

I think we'd need to add code to treat the nil UUID specially if we went this route - i.e. treat it as not having a UUID. But if we don't just want to fail in this case, perhaps it would be better to find/port some code.

[Richard Boulton]

I think raising a DatabaseCreateError if UuidCreate fails would be good enough for now. I'm attaching a cleaned up patch, based on Charlie's latest patch, which will hopefully fix this. Charlie - please test this patch against xapian HEAD on windows.

[Richard Boulton]

Improved patch, based on patch26.

[Charlie Hull]

This compiles perfectly.

[Richard Boulton]

Cool - but do the tests pass with it? (excluding ones that you know fail for other reasons).

[Charlie Hull]

Yes, everything passes until you get to the failures already reported in #312....although there's a slight difference, as the failures don't start until a few tests later. See #312 for details.

[Richard Boulton]

Patch committed.

[Olly Betts]

A few cleanups:

• Use named constants instead of "magic numbers".
• Fix a few minor non-standard formatting issues.
• throw std::bad_alloc() if UuidToString() fails.
• #include <cstring> since we use functions from it (memset, etc.)
• Mark the exceptional cases as rare().
• Don't cast away const.
• Make copying the result of UuidToString() robust so it can never overflow the supplied 37 byte buffer even if UuidToString() misbehaves.

Can you test the shortly-to-be-attached patch?

[Olly Betts]

cleanup patch

[Charlie Hull]

Yes, this compiles and passes tests (assuming #312 is something different).

[Olly Betts]

Thanks, applied.