Opened 16 years ago

Closed 16 years ago

#303 closed defect (fixed)

UUID functions missing on Windows

Reported by: Charlie Hull Owned by: Richard Boulton
Priority: normal Milestone: 1.1.0
Component: Backend-Flint Version: SVN trunk
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Operating System: Microsoft Windows

Description

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

Attachments (4)

patch24.patch (12.2 KB ) - added by Charlie Hull 16 years ago.
Patch to add uuid functions
patch26.patch (12.3 KB ) - added by Charlie Hull 16 years ago.
Better patch adding UUID functions
windows_uuid.patch (7.4 KB ) - added by Richard Boulton 16 years ago.
Improved patch, based on patch26.
win32-uuid-cleanup.patch (2.1 KB ) - added by Olly Betts 16 years ago.
cleanup patch

Download all attachments as: .zip

Change History (23)

by Charlie Hull, 16 years ago

Attachment: patch24.patch added

Patch to add uuid functions

comment:1 by Richard Boulton, 16 years ago

Owner: changed from Olly Betts to Richard Boulton
Status: newassigned

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.)

comment:2 by Olly Betts, 16 years ago

/* 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".

comment:3 by Olly Betts, 16 years ago

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 ")").

comment:4 by Olly Betts, 16 years ago

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).

comment:5 by Olly Betts, 16 years ago

Milestone: 1.0.91.1.0

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

comment:6 by Richard Boulton, 16 years ago

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

comment:7 by Charlie Hull, 16 years ago

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

by Charlie Hull, 16 years ago

Attachment: patch26.patch added

Better patch adding UUID functions

comment:8 by Olly Betts, 16 years ago

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...)

comment:9 by Olly Betts, 16 years ago

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...

comment:10 by Charlie Hull, 16 years ago

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.

comment:11 by Olly Betts, 16 years ago

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.

comment:12 by Richard Boulton, 16 years ago

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.

by Richard Boulton, 16 years ago

Attachment: windows_uuid.patch added

Improved patch, based on patch26.

comment:13 by Charlie Hull, 16 years ago

This compiles perfectly.

comment:14 by Richard Boulton, 16 years ago

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

comment:15 by Charlie Hull, 16 years ago

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.

comment:16 by Richard Boulton, 16 years ago

Resolution: fixed
Status: assignedclosed

Patch committed.

comment:17 by Olly Betts, 16 years ago

Resolution: fixed
Status: closedreopened

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?

by Olly Betts, 16 years ago

Attachment: win32-uuid-cleanup.patch added

cleanup patch

comment:18 by Charlie Hull, 16 years ago

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

comment:19 by Olly Betts, 16 years ago

Resolution: fixed
Status: reopenedclosed

Thanks, applied.

Note: See TracTickets for help on using tickets.