On Fri, May 14, 2010 at 12:47:06PM -0700, Jamey Sharp wrote: > On Fri, May 14, 2010 at 12:40 PM, Jeremy Huddleston <[email protected]> > wrote: > > Thanks Jamey! > > > > Reviewed-by: Jeremy Huddleston <[email protected]> > > No problem! Out of curiousity, did you just review the patch, or also > test it? I'd be inclined to add a tested-by line as well for you if > you did, in fact, test. :-) > > Also I'm really hoping Peter or somebody else who knows the input bits > will review this before it's merged. I'm confident it fixes the > symptom, and I'm pretty sure the patch isn't outright wrong, but there > might be something better to do?
The calloc part is certainly correct, the other hunk is problematic. If you return here, the device is marked as initialized but no presence event is sent. Then, when the device is removed, a DeviceRemoved event will be sent for a device that was never visible on the protocol. Note that clients must be able to handle this anyway because given the right timing, this could happen under normal circumstances. However to stay symmetrical in the server, the alternative would be just do ret = pScreen->DeviceCursorInitialize(dev, pScreen); This way we send out the DeviceAdded event (which makes sense, given that we've also asked the driver to initalize everything) and then rely on the caller of ActivateDevice to call RemoveDevice for cleanup to undo the damage. I think a BadAlloc would be better to return code than BadImplementation though. This path is called when creating new master devices and there's no reason they couldn't fail for various reasons with the server happily continuing to run. (Of course, that would need the matching checks in Xi/xichangehierarchy.c, where we just ignore the ActivateDevice return values and pretend life is perfect. That'd be a follow-up patch though.) so, given that change Reviewed-by: Peter Hutterer <[email protected]> and one nitpick - please use spaces here, ActivateDevice is (surprisingly) consistent with spaces-only indentation. Cheers, Peter _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
