Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Apache: Dev

DAV Provider Patch

  

 
Apache dev RSS feed   Index | Next | Previous | View Threaded


brian at brianfrance

Sep 21, 2009, 6:30 AM

Post #1 of 13 (1235 views)
Permalink
DAV Provider Patch
Next up is the dav providers patch. Currently there is no way for dav
modules to get access to the filename or the the request_rec. A dav
module would need to check the filename to see if needs to enable acls
or other options. A dav module would also need the request_rec for
checking options via the r->per_dir_config (like mod_dav_acl needs to
know what the principle path and location are set to). This can be
done one of two ways.

This patch adds two hooks functions to the dav_hooks_repository
struct, get_request_rec and get_pathname:

http://www.brianfrance.com/software/apache/dav/dav-provider-1.diff

it also patches mod_dav_fs to support these two functions. The other
option would be to only add the get_request_rec to the struct like this:

http://www.brianfrance.com/software/apache/dav/dav-provider-2.diff

and have a module just use r->filename to get the filename instead of
adding another function.

I believe this is the first patch that will break binary compatibility
because it adds a function pointer to the middle of the struct. I
believe binary compatibility could be retained if we add the function
pointers to the end of the struct instead of in the middle. Moving
the function could be part of the back porting patch to 2.2, but leave
it as is in 2.3+.

Both of these patches also fix an issue where the definition of
dav_hooks_repository_fs in modules/dav/fs/repos.c doesn't have a NULL
value for the ctx (broken in HEAD and 2.2.x) at the end of the struct.

The patches are based on Jari's httpd-2.2.8-ju.patch patch.

Comments, questions, problems?

Brian


minfrin at sharp

Sep 21, 2009, 7:15 AM

Post #2 of 13 (1195 views)
Permalink
Re: DAV Provider Patch [In reply to]
Brian J. France wrote:

> I believe this is the first patch that will break binary compatibility
> because it adds a function pointer to the middle of the struct. I
> believe binary compatibility could be retained if we add the function
> pointers to the end of the struct instead of in the middle. Moving the
> function could be part of the back porting patch to 2.2, but leave it as
> is in 2.3+.

Would it be possible to move the function pointers to the end of the
struct for httpd-trunk as well? Breaking binary compatibility is to be
avoided if it can be, even on trunk.

Regards,
Graham
--
Attachments: smime.p7s (3.22 KB)


brian at brianfrance

Sep 21, 2009, 7:44 AM

Post #3 of 13 (1198 views)
Permalink
Re: DAV Provider Patch [In reply to]
On Sep 21, 2009, at 10:15 AM, Graham Leggett wrote:

> Brian J. France wrote:
>
>> I believe this is the first patch that will break binary
>> compatibility
>> because it adds a function pointer to the middle of the struct. I
>> believe binary compatibility could be retained if we add the function
>> pointers to the end of the struct instead of in the middle. Moving
>> the
>> function could be part of the back porting patch to 2.2, but leave
>> it as
>> is in 2.3+.
>
> Would it be possible to move the function pointers to the end of the
> struct for httpd-trunk as well? Breaking binary compatibility is to be
> avoided if it can be, even on trunk.


Sure. Which method would be preferred? Having two hooks or just one
and use the request_rec to get the filename?

Brian


poirier at pobox

Sep 21, 2009, 8:28 AM

Post #4 of 13 (1197 views)
Permalink
Re: DAV Provider Patch [In reply to]
"Brian J. France" <brian [at] brianfrance> writes:

> On Sep 21, 2009, at 10:15 AM, Graham Leggett wrote:
>
>> Brian J. France wrote:
>>
>>> I believe this is the first patch that will break binary
>>> compatibility
>>> because it adds a function pointer to the middle of the struct. I
>>> believe binary compatibility could be retained if we add the function
>>> pointers to the end of the struct instead of in the middle. Moving
>>> the
>>> function could be part of the back porting patch to 2.2, but leave
>>> it as
>>> is in 2.3+.
>>
>> Would it be possible to move the function pointers to the end of the
>> struct for httpd-trunk as well? Breaking binary compatibility is to be
>> avoided if it can be, even on trunk.
>
>
> Sure. Which method would be preferred? Having two hooks or just one
> and use the request_rec to get the filename?

As long as the filename can be gotten trivially from the request rec,
I'd say keep things simple and just add the one hook.

--
Dan Poirier <poirier [at] pobox>


ruediger.pluem at vodafone

Sep 21, 2009, 8:34 AM

Post #5 of 13 (1194 views)
Permalink
RE: DAV Provider Patch [In reply to]
> -----Original Message-----
> From: Dan Poirier
> Sent: Montag, 21. September 2009 17:28
> To: dev [at] httpd
> Subject: Re: DAV Provider Patch
>
> "Brian J. France" <brian [at] brianfrance> writes:
>
> > On Sep 21, 2009, at 10:15 AM, Graham Leggett wrote:
> >
> >> Brian J. France wrote:
> >>
> >>> I believe this is the first patch that will break binary
> >>> compatibility
> >>> because it adds a function pointer to the middle of the struct. I
> >>> believe binary compatibility could be retained if we add
> the function
> >>> pointers to the end of the struct instead of in the
> middle. Moving
> >>> the
> >>> function could be part of the back porting patch to 2.2, but leave
> >>> it as
> >>> is in 2.3+.
> >>
> >> Would it be possible to move the function pointers to the
> end of the
> >> struct for httpd-trunk as well? Breaking binary
> compatibility is to be
> >> avoided if it can be, even on trunk.
> >
> >
> > Sure. Which method would be preferred? Having two hooks
> or just one
> > and use the request_rec to get the filename?
>
> As long as the filename can be gotten trivially from the request rec,
> I'd say keep things simple and just add the one hook.

+1.

Regards

Rüdiger


brian at brianfrance

Oct 6, 2009, 2:15 PM

Post #6 of 13 (1138 views)
Permalink
Re: DAV Provider Patch [In reply to]
Sorry for the delay in response to this, life got in the way.

I have updated the patch here:

http://www.brianfrance.com/software/apache/dav/dav-provider-3.diff

This patch doesn't break binary compatibility (adds the functions to
the end of the struct) and adds both get_request_rec and
get_pathname. While in most cases you can pull pathname from the
request_rec, how would you get the pathname from a mod_dav_fs_db type
module? Should mod_dav_fs_db update r->filename or should we keep the
get_pathname function in the provider struct?

Either way works for me, just happen to have a discussion at work
about writing a custom mod_dav_fs module and thought of this patch case.

Thoughts?

Brian

On Sep 21, 2009, at 11:34 AM, Plüm, Rüdiger, VF-Group wrote:
>> -----Original Message-----
>> From: Dan Poirier
>> Sent: Montag, 21. September 2009 17:28
>> To: dev [at] httpd
>> Subject: Re: DAV Provider Patch
>>
>> "Brian J. France" <brian [at] brianfrance> writes:
>>
>>> On Sep 21, 2009, at 10:15 AM, Graham Leggett wrote:
>>>
>>>> Brian J. France wrote:
>>>>
>>>>> I believe this is the first patch that will break binary
>>>>> compatibility
>>>>> because it adds a function pointer to the middle of the struct. I
>>>>> believe binary compatibility could be retained if we add
>> the function
>>>>> pointers to the end of the struct instead of in the
>> middle. Moving
>>>>> the
>>>>> function could be part of the back porting patch to 2.2, but leave
>>>>> it as
>>>>> is in 2.3+.
>>>>
>>>> Would it be possible to move the function pointers to the
>> end of the
>>>> struct for httpd-trunk as well? Breaking binary
>> compatibility is to be
>>>> avoided if it can be, even on trunk.
>>>
>>>
>>> Sure. Which method would be preferred? Having two hooks
>> or just one
>>> and use the request_rec to get the filename?
>>
>> As long as the filename can be gotten trivially from the request rec,
>> I'd say keep things simple and just add the one hook.
>
> +1.
>
> Regards
>
> Rüdiger
>


nick at webthing

Oct 6, 2009, 4:40 PM

Post #7 of 13 (1137 views)
Permalink
Re: DAV Provider Patch [In reply to]
On 6 Oct 2009, at 22:15, Brian J. France wrote:

> Sorry for the delay in response to this, life got in the way.
>
> I have updated the patch here:
>
> http://www.brianfrance.com/software/apache/dav/dav-provider-3.diff
>
> This patch doesn't break binary compatibility (adds the functions to
> the end of the struct) and adds both get_request_rec and
> get_pathname. While in most cases you can pull pathname from the
> request_rec, how would you get the pathname from a mod_dav_fs_db
> type module? Should mod_dav_fs_db update r->filename or should we
> keep the get_pathname function in the provider struct?
>
> Either way works for me, just happen to have a discussion at work
> about writing a custom mod_dav_fs module and thought of this patch
> case.

My recollection of hacking at mod_dav is that I wanted to make some
similar
changes, but I was in two minds whether patching the API like this was
the
right solution (certainly getting the request_rec), or whether it
wanted a
deeper-level redesign.

Is this API change sufficient for your app? And if not, how much
more is there to come?

--
Nick Kew


brian at brianfrance

Oct 6, 2009, 6:57 PM

Post #8 of 13 (1136 views)
Permalink
Re: DAV Provider Patch [In reply to]
On Oct 6, 2009, at 7:40 PM, Nick Kew wrote:

>
> On 6 Oct 2009, at 22:15, Brian J. France wrote:
>
>> Sorry for the delay in response to this, life got in the way.
>>
>> I have updated the patch here:
>>
>> http://www.brianfrance.com/software/apache/dav/dav-provider-3.diff
>>
>> This patch doesn't break binary compatibility (adds the functions
>> to the end of the struct) and adds both get_request_rec and
>> get_pathname. While in most cases you can pull pathname from the
>> request_rec, how would you get the pathname from a mod_dav_fs_db
>> type module? Should mod_dav_fs_db update r->filename or should we
>> keep the get_pathname function in the provider struct?
>>
>> Either way works for me, just happen to have a discussion at work
>> about writing a custom mod_dav_fs module and thought of this patch
>> case.
>
> My recollection of hacking at mod_dav is that I wanted to make some
> similar
> changes, but I was in two minds whether patching the API like this
> was the
> right solution (certainly getting the request_rec), or whether it
> wanted a
> deeper-level redesign.
>
> Is this API change sufficient for your app? And if not, how much
> more is there to come?


There are two more patches left. One is for handling ETags and will
patch both dav and http_etag.c to handle usec timestamps in the
ETags. The second one is the main ACL support patch to the dav stuff.

After that, all the rest of mod_dav_acl, mod_caldav and mod_carddav
will work with out any patches to httpd/dav.

Brian


minfrin at sharp

Oct 7, 2009, 2:37 PM

Post #9 of 13 (1127 views)
Permalink
Re: DAV Provider Patch [In reply to]
Brian J. France wrote:

> Sorry for the delay in response to this, life got in the way.
>
> I have updated the patch here:
>
> http://www.brianfrance.com/software/apache/dav/dav-provider-3.diff
>
> This patch doesn't break binary compatibility (adds the functions to the
> end of the struct) and adds both get_request_rec and get_pathname.
> While in most cases you can pull pathname from the request_rec, how
> would you get the pathname from a mod_dav_fs_db type module? Should
> mod_dav_fs_db update r->filename or should we keep the get_pathname
> function in the provider struct?
>
> Either way works for me, just happen to have a discussion at work about
> writing a custom mod_dav_fs module and thought of this patch case.

Just a quick check - am I right in understanding that the get_pathname
function below is an oversight?

Index: modules/dav/main/mod_dav.h
===================================================================
--- modules/dav/main/mod_dav.h (revision 822497)
+++ modules/dav/main/mod_dav.h (working copy)
@@ -1940,6 +1940,12 @@
** then this field may be used. In most cases, it will just be NULL.
*/
void *ctx;
+
+ /* return request record */
+ request_rec * (*get_request_rec)(const dav_resource *resource);
+
+ /* return path */
+ const char * (*get_pathname)(const dav_resource *resource);
};

Regards,
Graham
--
Attachments: smime.p7s (3.22 KB)


brian at brianfrance

Oct 7, 2009, 3:18 PM

Post #10 of 13 (1129 views)
Permalink
Re: DAV Provider Patch [In reply to]
On Oct 7, 2009, at 5:37 PM, Graham Leggett wrote:

> Brian J. France wrote:
>
>> Sorry for the delay in response to this, life got in the way.
>>
>> I have updated the patch here:
>>
>> http://www.brianfrance.com/software/apache/dav/dav-provider-3.diff
>>
>> This patch doesn't break binary compatibility (adds the functions
>> to the
>> end of the struct) and adds both get_request_rec and get_pathname.
>> While in most cases you can pull pathname from the request_rec, how
>> would you get the pathname from a mod_dav_fs_db type module? Should
>> mod_dav_fs_db update r->filename or should we keep the get_pathname
>> function in the provider struct?
>>
>> Either way works for me, just happen to have a discussion at work
>> about
>> writing a custom mod_dav_fs module and thought of this patch case.
>
> Just a quick check - am I right in understanding that the get_pathname
> function below is an oversight?
>
> Index: modules/dav/main/mod_dav.h
> ===================================================================
> --- modules/dav/main/mod_dav.h (revision 822497)
> +++ modules/dav/main/mod_dav.h (working copy)
> @@ -1940,6 +1940,12 @@
> ** then this field may be used. In most cases, it will just be
> NULL.
> */
> void *ctx;
> +
> + /* return request record */
> + request_rec * (*get_request_rec)(const dav_resource *resource);
> +
> + /* return path */
> + const char * (*get_pathname)(const dav_resource *resource);
> };

Depends.

Should a mod_dav_fs type module (like mod_dav_fs_database) update r-
>filename so other modules like mod_dav_acl could use the filename
from the request_rec.
Or should mod_dav_acl use a hook function to get the pathname because
r->filename would not be set correctly since that is a path on disk in
the case of mod_dav_fs_database?

My patch (version 3) left the get_pathname hook with the assumption
that r->filename should not be used and instead a hook should be used.

Brian


minfrin at sharp

Oct 9, 2009, 2:46 PM

Post #11 of 13 (1117 views)
Permalink
Re: DAV Provider Patch [In reply to]
Brian J. France wrote:

> Depends.
>
> Should a mod_dav_fs type module (like mod_dav_fs_database) update
> r->filename so other modules like mod_dav_acl could use the filename
> from the request_rec.
> Or should mod_dav_acl use a hook function to get the pathname because
> r->filename would not be set correctly since that is a path on disk in
> the case of mod_dav_fs_database?
>
> My patch (version 3) left the get_pathname hook with the assumption that
> r->filename should not be used and instead a hook should be used.

Thanks for this, committed to httpd-trunk in r823703.

Regards,
Graham
--
Attachments: smime.p7s (3.22 KB)


brian at brianfrance

Oct 22, 2009, 9:11 AM

Post #12 of 13 (1033 views)
Permalink
Re: DAV Provider Patch [In reply to]
On Oct 9, 2009, at 5:46 PM, Graham Leggett wrote:
> Brian J. France wrote:
>
>> Depends.
>>
>> Should a mod_dav_fs type module (like mod_dav_fs_database) update
>> r->filename so other modules like mod_dav_acl could use the filename
>> from the request_rec.
>> Or should mod_dav_acl use a hook function to get the pathname because
>> r->filename would not be set correctly since that is a path on disk
>> in
>> the case of mod_dav_fs_database?
>>
>> My patch (version 3) left the get_pathname hook with the assumption
>> that
>> r->filename should not be used and instead a hook should be used.
>
> Thanks for this, committed to httpd-trunk in r823703.


Graham,

Could you commit this as well.

http://www.brianfrance.com/software/apache/dav/fixup.diff

It is a clean up of r823703, fixes bad struct declaration and changes
comments on functions.

Thanks,

Brian


minfrin at sharp

Nov 30, 2009, 2:41 PM

Post #13 of 13 (890 views)
Permalink
Re: DAV Provider Patch [In reply to]
Brian J. France wrote:

> Graham,
>
> Could you commit this as well.
>
> http://www.brianfrance.com/software/apache/dav/fixup.diff
>
> It is a clean up of r823703, fixes bad struct declaration and changes
> comments on functions.

I see this was done in r830284.

Regards,
Graham
--

Apache dev RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.