Hey Emil,
Emil Velikov wrote:
On 30 October 2015 at 11:28, Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
Hello Emil,
Emil Velikov wrote:
On 30 October 2015 at 11:16, Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
Hello Hyungwon,
first of all thanks for reviewing the series!
Hyungwon Hwang wrote:
On Tue, 22 Sep 2015 17:54:55 +0200 Tobias Jakobi tjakobi@math.uni-bielefeld.de wrote:
- evhandler->evctx.base.version = DRM_EVENT_CONTEXT_VERSION;
- evhandler->evctx.version = EXYNOS_EVENT_CONTEXT_VERSION;
The versions must be set not using XXX_EVENT_CONTEXT_VERSION. After the versions are bumped, the event will contains wrong version info.
Hmm, I don't see how this is true. Both DRM_EVENT_CONTEXT_VERSION and EXYNOS_EVENT_CONTEXT_VERSION come from the public libdrm header. If the version in the public header is bumped, then it's also bumped here. So I don't see the issue.
The issue is that the public header defines the interface available, while you set the version that you implement. Currently those match, but if/when we expand the API (bumping the version in the header) and rebuild your program we will crash.
Hmm, I'm still not sure I understand this. Do you mean rebuilding the tests out-of-tree and then running them against a newer/older libdrm version?
Because from my understanding the tests are always build together with libdrm anyway. Or am I misunderstanding here something?
We're not talking about building out of tree or anything like that here. The following example should illustrate what I have in mind. Greatly appreciated if you can explain it in your own words.
Currently
- xf86drm.h
#define DRM_EVENT_CONTEXT_VERSION 2 struct drmEventContext { int version; ... void (*page_flip_handler)(...); }
- user
struct foo { .version = ...VERSION // 2 ... }
API bump
- xf86drm.h
#define DRM_EVENT_CONTEXT_VERSION 3 struct drmEventContext { int version; ... void (*page_flip_handler)(...); void (*page_flip_handler2)(...); }
- user (hasn't been updated, only rebuilt)
struct foo { .version = ...VERSION // 3 ... .page_flip_handler2 = 0 // ... or garbage. }
- xf86drmMode.c
int drmHandleEvent(...) { ... if (foo.version >= 3) foo.page_flip_handler2(); // boom ! else foo.page_flip_handler(); ... }
Also worth mentioning is that the issue is rather wide spread rather than limited to libdrm :'(
OK, I see what you mean. However this shouldn't happen as long as the user properly zero initializes the event context structures, right?
Before you ask - yes current libdrm users are not doing the right thing and should be updated one of these days.
Maybe a dumb question, but what would be the right thing to do in may case.
Define my own MY_XZY_EVENT_CONTEXT_VERSION and use these?
No need for extra defines imho. Just set the versions to 2 and 1 for evctx.base.version and evctx.version respectively.
OK, going to do this then. In any case, can the user assume that the event structures only "grow", in the sense that new fields are added to it?
With best wishes, Tobias
Glad to see some of the Samsung/Exynos people looking this way :-)
Regards, Emil