Hi Arun :-)
On Mon, Apr 25, 2016 at 04:30:22PM +0530, Arun Raghavan wrote:
> On Fri, 2016-04-15 at 23:06 +0200, Ahmed S. Darwish wrote:
...
> > @@ -482,6 +487,7 @@ static void setup_complete_callback(pa_pdispatch *pd, uint32_t command, uint32_t
> > Â Â Â Â Â Â Â Â Â case PA_CONTEXT_AUTHORIZING: {
> > Â Â Â Â Â Â Â Â Â Â Â Â Â pa_tagstruct *reply;
> > Â Â Â Â Â Â Â Â Â Â Â Â Â bool shm_on_remote = false;
> > +Â Â Â Â Â Â Â Â Â Â Â Â bool memfd_on_remote = false;
> > Â
> > Â Â Â Â Â Â Â Â Â Â Â Â Â if (pa_tagstruct_getu32(t, &c->version) < 0 ||
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â !pa_tagstruct_eof(t)) {
> > @@ -500,7 +506,15 @@ static void setup_complete_callback(pa_pdispatch *pd, uint32_t command, uint32_t
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â not. */
> > Â Â Â Â Â Â Â Â Â Â Â Â Â if (c->version >= 13) {
> > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â shm_on_remote = !!(c->version & 0x80000000U);
> > -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â c->version &= 0x7FFFFFFFU;
> > +
> > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* Starting with protocol version 31, the second MSB of the version
> > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â * tag reflects whether memfd is supported on the other PA end. */
> > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (c->version >= 31)
>
> You should be using (c->version &Â 0x40000000U) for the comparison to
> get the actual protocol version.
>
> Not a comment for this patch, but we should probably introduce some
> macros to deal with this flag setting to keep things cleaner.
>
Hmm.. if there are any flags in the most-significant bytes, then
this check shall pass and we can succesfully check for memfd
availability by checking the second MSB. That's why current patch
works as expected (that is, correctly deduce memfd availability
or non-availability) with both < 31 and > 31 PA endpoints. That's
the case too because the two most-significant bytes are force
cleared afterwards.
Nonetheless, of course what you're proposing is the more correct
logic. So if this is the only problem, let's merge and I'll create
a follow-up patch this week that introduces the desired macros and
fixes up this version-check issue in the process.
> > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â memfd_on_remote = !!(c->version & 0x40000000U);
> > +
> > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* Reserve the two most-significant _bytes_ of the version tag
> > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â * for flags. */
> > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â c->version &= 0x0000FFFFU;
> > Â Â Â Â Â Â Â Â Â Â Â Â Â }
> > Â
> > Â Â Â Â Â Â Â Â Â Â Â Â Â pa_log_debug("Protocol version: remote %u, local %u", c->version, PA_PROTOCOL_VERSION);
Thanks!
--
Darwish
http://darwish.chasingpointers.com
