Re: [Xen-devel] [PATCH 09 of 30] tools: blktap2: copy xenstore/hashtable.h into blktap2

[Shriram Rajagopalan <rshriram@xxxxxxxxx>]

On Mon, Mar 21, 2011 at 2:56 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx> wrote:

On Mon, 2011-03-21 at 20:47 +0000, Shriram Rajagopalan wrote:
> On Mon, Mar 21, 2011 at 11:03 AM, Ian Jackson
> <Ian.Jackson@xxxxxxxxxxxxx> wrote:
>         Ian Campbell writes ("[Xen-devel] [PATCH 09 of 30] tools:
>         blktap2: copy xenstore/hashtable.h into blktap2"):
>         > tools: blktap2: copy xenstore/hashtable.h into blktap2
>         >
>         > hashtable.c has already been copied, forked and modified,
>         there
>         > doesn't seem much point in avoiding the same for the header
>         until
>         > someone feels motivated to properly refactor.
>
>
>         Before we make this any worse, perhaps we should at least
>         leave a
>         comment in every copy of hashtable.c referring to every other
>         clone ?
>
>         Ian.
>
>
>         _______________________________________________
>         Xen-devel mailing list
>         Xen-devel@xxxxxxxxxxxxxxxxxxx
>         http://lists.xensource.com/xen-devel
>
> copying hashtable.h alone wont suffice.
> hashtable_itr.h in blktap2/drivers includes hashtable_private.h in
> xenstore/

The commit copies both, although I forgot to update the description to
say so.

> And a diff -wu blktap2/drivers/hashtable.c xenstore/hashtable.c shows
> no functional difference between the two versions.

Well, there are definitely some functional differences, they may well be
trivial I suppose but determining that did not look immediately trivial
so I didn't dig deeper.

answered below

If someone wants to merge the two versions and validate that it works
for both usages I would be happy to have that patch instead of this one,
or to have this one in the interim while that gets sorted out.

>   How about pulling the hashtable.h, hashtable.c, hashtable_private.h
> from xenstore/ and hashtable_itr.[h,c], hashtable_utility.[h,c] from
> blktap2/drivers/ into one common separate area, like utils/ or
> something.

Well, you can't just blindly take the blktap2 version since the xenstore
version has had changes made to it since blktap2 forked its version off
so you'd need to at least do a merge up pass first. In particular
9129:1c46091df7ce removed the use of floating point which seems like a
useful change. 17816:189597fbb882 includes BSD portability fixes which
are important for the xenstore usage (and 17819:57b8c74c35ef includes an
incremental fix). I don't know if that accounts for all the differences
or not.

I had already looked at those two change sets
9129: They are functionally same. Basically the blktap2 version
uses 0.65 for specifying the load factor, while xenstore version uses 65 (used as %)
for the load factor.
17816: changing mallocs to callocs.

xenstore and blktap2 have no common dependency where this shared code
would sanely live, I suppose tools/utils or tools/hashtable is as good
as anywhere, although I'm not sure I think the world really needs
libxenhashtable.so...

that though is true. For this very reason alone, I think its simpler to have these
two clones around than to refactor into a tools/util folder.

> Also, I suggest moving all code from hashtable_itr.c and
> hashtable_utility.c into hashtable.c (and adding the appropriate
> declarations to hashtable.h). The copyrights and attributions in the
> files seem to be the same. We could get rid of all the clutter and
> have just 3 files related to hashtable, common to both xenstore and
> blktap2/drivers/.

Seems ok to me.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel