[ovs-dev,ovn] logical-fields: fix memory leak caused by initialize ovnfield_by_name twice

ovnfield_by_name is hash of strings which is used to quickly find
field by name. This hash is initialized from ovn_init_symtab(). In case
the latter function is called multiple times then also ovnfield_by_name is
initialized multiple times but without freeing previously allocated
memory resources what cause memory leaks.  This actually happens in
ovn-controller which calls ovn_init_symtab() function twice, once from
ofctrl.c and the other time from lflow.c files.

Problem was solved by initializing ovnfield_by_name entity only once
with the help of ovsthread_once_XXXX functionality.

Problem was reported by valgrind with flood of messages similar to this one:

==5999== 47 (32 direct, 15 indirect) bytes in 1 blocks are definitely lost in loss record 86 of 102
==5999==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5999==    by 0x50635D: xmalloc (util.c:138)
==5999==    by 0x4F6513: shash_add_nocopy__ (shash.c:109)
==5999==    by 0x4F6585: shash_add_nocopy (shash.c:121)
==5999==    by 0x4F65BD: shash_add (shash.c:129)
==5999==    by 0x4F6602: shash_add_once (shash.c:136)
==5999==    by 0x4395B7: ovn_init_symtab (logical-fields.c:261)
==5999==    by 0x406C91: main (ovn-controller.c:1750)

Signed-off-by: Damijan Skvarc <damjan.skvarc@gmail.com>
---
 lib/logical-fields.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

Hi Damijan,

Thanks for finding the memory leak and patching it. To me, 
ovnfield_by_name is a really strangely handled structure. There is an 
explicit function to destroy the ovnfield_by_name structure, but its 
creation is a side effect from ovn_symtab_init. The result is that 
lflow.c calls ovn_symtab_init() and ovn_destroy_ovnfields(). But ofctrl 
only calls ovn_symtab_init(). It doesn't call ovn_destroy_ovnfields() 
because it would result in double-freeing ovnfield_by_name.

This lack of symmetry shows a poor design. Your patch fixes the memory 
leak, but it doesn't fix the lack of symmetry, and it still allows for 
potential double-freeing.

I have a couple of suggestions for a better design:

1) Instead of calling ovn_symbtab_init() in multiple places, call it 
once in ovn_controller's main() function. Then when ovn_controller 
exits, call ovn_destroy_ovnfields(). Then, pass the created symtab as a 
parameter when it is necessary to. This works because the symtabs 
created by lflow.c and ofctrl.c are identical, and their use of the 
symtabs occurs in the same thread.

2) Initialize ovnfield_by_name in a separate function from 
ovn_symtab_init(). You can call this new ovnfield_by_name_init() 
function in ovn-controller's main function and then call the already 
existing ovn_destroy_ovnfields() function when ovn-controller exits. 
This works because ovnfield_by_name does not rely on any data from the 
created symtab. It exists completely independently.

What do you think about this?

On 2/27/20 8:17 AM, Damijan Skvarc wrote:
> ovnfield_by_name is hash of strings which is used to quickly find
> field by name. This hash is initialized from ovn_init_symtab(). In case
> the latter function is called multiple times then also ovnfield_by_name is
> initialized multiple times but without freeing previously allocated
> memory resources what cause memory leaks.  This actually happens in
> ovn-controller which calls ovn_init_symtab() function twice, once from
> ofctrl.c and the other time from lflow.c files.
> 
> Problem was solved by initializing ovnfield_by_name entity only once
> with the help of ovsthread_once_XXXX functionality.
> 
> Problem was reported by valgrind with flood of messages similar to this one:
> 
> ==5999== 47 (32 direct, 15 indirect) bytes in 1 blocks are definitely lost in loss record 86 of 102
> ==5999==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==5999==    by 0x50635D: xmalloc (util.c:138)
> ==5999==    by 0x4F6513: shash_add_nocopy__ (shash.c:109)
> ==5999==    by 0x4F6585: shash_add_nocopy (shash.c:121)
> ==5999==    by 0x4F65BD: shash_add (shash.c:129)
> ==5999==    by 0x4F6602: shash_add_once (shash.c:136)
> ==5999==    by 0x4395B7: ovn_init_symtab (logical-fields.c:261)
> ==5999==    by 0x406C91: main (ovn-controller.c:1750)
> 
> Signed-off-by: Damijan Skvarc <damjan.skvarc@gmail.com>
> ---
>   lib/logical-fields.c | 25 +++++++++++++++++++------
>   1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/logical-fields.c b/lib/logical-fields.c
> index 25ace58..569e31e 100644
> --- a/lib/logical-fields.c
> +++ b/lib/logical-fields.c
> @@ -254,12 +254,8 @@ ovn_init_symtab(struct shash *symtab)
>       expr_symtab_add_field(symtab, "sctp.src", MFF_SCTP_SRC, "sctp", false);
>       expr_symtab_add_field(symtab, "sctp.dst", MFF_SCTP_DST, "sctp", false);
>   
> -    shash_init(&ovnfield_by_name);
> -    for (int i = 0; i < OVN_FIELD_N_IDS; i++) {
> -        const struct ovn_field *of = &ovn_fields[i];
> -        ovs_assert(of->id == i); /* Fields must be in the enum order. */
> -        shash_add_once(&ovnfield_by_name, of->name, of);
> -    }
> +    ovn_init_ovnfields();
> +
>       expr_symtab_add_ovn_field(symtab, "icmp4.frag_mtu", OVN_ICMP4_FRAG_MTU);
>   }
>   
> @@ -295,3 +291,20 @@ ovn_destroy_ovnfields(void)
>   {
>       shash_destroy(&ovnfield_by_name);
>   }
> +
> +void
> +ovn_init_ovnfields(void)
> +{
> +    static struct ovsthread_once field_by_name_once =
> +        OVSTHREAD_ONCE_INITIALIZER;
> +
> +    if (ovsthread_once_start(&field_by_name_once)) {
> +       shash_init(&ovnfield_by_name);
> +       for (int i = 0; i < OVN_FIELD_N_IDS; i++) {
> +           const struct ovn_field *of = &ovn_fields[i];
> +           ovs_assert(of->id == i); /* Fields must be in the enum order. */
> +           shash_add_once(&ovnfield_by_name, of->name, of);
> +       }
> +       ovsthread_once_done(&field_by_name_once);
> +    }
> +}
>

Hi Mark

and thanks you made review of suggested patch.

just a few words of myself...
I don't work professionally on this project, I started to look into this
code in my free hours just to satisfy my curiosity about how other people
work. I am following a couple of other projects just to learn about
different processes, tools and to keep my brains in good condition while
studying some others work. And if you get some things/knowledge it is also
kind to give back something. In case of ovs/ovn I found a nice suite of
automatic tests,. Unfortunately, by running them under valgrind control the
apparent reports become useless because of flood of memory leaks. Mostly,
these are small non-important leaks, however in a flood of duplicated
reports you can miss some serious issue. And here I found my personal goal
what I can give back. Unfortunately, I don't have  much knowledge about
networking, nor about original design. And if you don't have that knowledge
then from the experience I know it is better not to touch anything for what
you don't know why it has been designed for. So every my patch is done in a
way that nothing has been changed in a design, just memory leak in valgrind
reports disappears.


On Thu, Feb 27, 2020 at 10:24 PM Mark Michelson <mmichels@redhat.com> wrote:

> Hi Damijan,
>
> Thanks for finding the memory leak and patching it. To me,
> ovnfield_by_name is a really strangely handled structure. There is an
> explicit function to destroy the ovnfield_by_name structure, but its
> creation is a side effect from ovn_symtab_init. The result is that
> lflow.c calls ovn_symtab_init() and ovn_destroy_ovnfields(). But ofctrl
> only calls ovn_symtab_init(). It doesn't call ovn_destroy_ovnfields()
> because it would result in double-freeing ovnfield_by_name.
>
>
yes, I noticed that asymmetry.

This lack of symmetry shows a poor design.
>
Your patch fixes the memory
> leak, but it doesn't fix the lack of symmetry, and it still allows for
> potential double-freeing.

I have a couple of suggestions for a better design:
>
> 1) Instead of calling ovn_symbtab_init() in multiple places, call it
> once in ovn_controller's main() function. Then when ovn_controller
> exits, call ovn_destroy_ovnfields(). Then, pass the created symtab as a
> parameter when it is necessary to. This works because the symtabs
> created by lflow.c and ofctrl.c are identical, and their use of the
> symtabs occurs in the same thread.
>
> 2) Initialize ovnfield_by_name in a separate function from
> ovn_symtab_init(). You can call this new ovnfield_by_name_init()
> function in ovn-controller's main function and then call the already
> existing ovn_destroy_ovnfields() function when ovn-controller exits.
> This works because ovnfield_by_name does not rely on any data from the
> created symtab. It exists completely independently.
>
> What do you think about this?
>

What is strange also is that (If I see correctly) the ovnfield_by_name is a
hash of one single entry.
I wouldn't use hash at all in this case. But since I don't know about the
design, probably some entries
were deleted in the past and only one remained or probably there is a plan
some more entries will be
added in the future. So let's assume we should not get rid of
ovnfield_by_name hash.

I found very similar functionality in meta_flow.c file (ovs). There
mf_by_name hash is more rational, it is filled with
plenty of strings. Also initialization of this hash is more rational. It is
initialized only if it is needed (initialization is hidden
in mf_from_name() and mf_from_name_len() functions and using the same
"pthread_once" approach as in my case). What is strange there is that there
is no destroy action for mf_by_name and valgrind does not report any leak
there. Probably test suite does not cover this functionality.

I believe some more alternatives are available, however to reduce the
number of changes I would vote for
- automatic and optional initialization similar as in meta_flow case
- to solve asymmetry issue I would propose atexit() approach, similar as it
is implemented in ovs/lib/stopwatch.c

btw, I didn't understand your first proposal. ovn_symbtab_init() function
need to be called twice since two different
symtab entities are initialized. Is this probably also a design issue that
we could live with single symtab?

thanks,
damijan

>
>

On Fri, Feb 28, 2020 at 4:11 PM Damijan Skvarc <damjan.skvarc@gmail.com> wrote:
>
> Hi Mark
>
> and thanks you made review of suggested patch.
>
> just a few words of myself...
> I don't work professionally on this project, I started to look into this
> code in my free hours just to satisfy my curiosity about how other people
> work. I am following a couple of other projects just to learn about
> different processes, tools and to keep my brains in good condition while
> studying some others work. And if you get some things/knowledge it is also
> kind to give back something. In case of ovs/ovn I found a nice suite of
> automatic tests,. Unfortunately, by running them under valgrind control the
> apparent reports become useless because of flood of memory leaks. Mostly,
> these are small non-important leaks, however in a flood of duplicated
> reports you can miss some serious issue. And here I found my personal goal
> what I can give back. Unfortunately, I don't have  much knowledge about
> networking, nor about original design. And if you don't have that knowledge
> then from the experience I know it is better not to touch anything for what
> you don't know why it has been designed for. So every my patch is done in a
> way that nothing has been changed in a design, just memory leak in valgrind
> reports disappears.
>
>
> On Thu, Feb 27, 2020 at 10:24 PM Mark Michelson <mmichels@redhat.com> wrote:
>
> > Hi Damijan,
> >
> > Thanks for finding the memory leak and patching it. To me,
> > ovnfield_by_name is a really strangely handled structure. There is an
> > explicit function to destroy the ovnfield_by_name structure, but its
> > creation is a side effect from ovn_symtab_init. The result is that
> > lflow.c calls ovn_symtab_init() and ovn_destroy_ovnfields(). But ofctrl
> > only calls ovn_symtab_init(). It doesn't call ovn_destroy_ovnfields()
> > because it would result in double-freeing ovnfield_by_name.
> >
> >
> yes, I noticed that asymmetry.
>
> This lack of symmetry shows a poor design.
> >
> Your patch fixes the memory
> > leak, but it doesn't fix the lack of symmetry, and it still allows for
> > potential double-freeing.
>
> I have a couple of suggestions for a better design:
> >
> > 1) Instead of calling ovn_symbtab_init() in multiple places, call it
> > once in ovn_controller's main() function. Then when ovn_controller
> > exits, call ovn_destroy_ovnfields(). Then, pass the created symtab as a
> > parameter when it is necessary to. This works because the symtabs
> > created by lflow.c and ofctrl.c are identical, and their use of the
> > symtabs occurs in the same thread.
> >
> > 2) Initialize ovnfield_by_name in a separate function from
> > ovn_symtab_init(). You can call this new ovnfield_by_name_init()
> > function in ovn-controller's main function and then call the already
> > existing ovn_destroy_ovnfields() function when ovn-controller exits.
> > This works because ovnfield_by_name does not rely on any data from the
> > created symtab. It exists completely independently.
> >
> > What do you think about this?
> >
>
> What is strange also is that (If I see correctly) the ovnfield_by_name is a
> hash of one single entry.
> I wouldn't use hash at all in this case. But since I don't know about the
> design, probably some entries
> were deleted in the past and only one remained or probably there is a plan
> some more entries will be
> added in the future. So let's assume we should not get rid of
> ovnfield_by_name hash.
>

This is the commit which added 'ovnfield_by_name' [1]. I only added one field
at the time. The commit message has more details. I think when working on it
I missed out on the fact that ofctrl.c also calls ovn_init_symtab().

I think we can address this issue by having a new function -
ovn_init_ovnfields()
which lflow.c would call. ovn_destroy_ovnfields() is called by lflow.c anyway.

What do you think about this ?


[1] - https://github.com/openvswitch/ovs/commit/086470cdbe66522a1cfff96eb68073e4176684be
Thank

Numan



> I found very similar functionality in meta_flow.c file (ovs). There
> mf_by_name hash is more rational, it is filled with
> plenty of strings. Also initialization of this hash is more rational. It is
> initialized only if it is needed (initialization is hidden
> in mf_from_name() and mf_from_name_len() functions and using the same
> "pthread_once" approach as in my case). What is  strange there is that there
> is no destroy action for mf_by_name and valgrind does not report any leak
> there. Probably test suite does not cover this functionality.
>
> I believe some more alternatives are available, however to reduce the
> number of changes I would vote for
> - automatic and optional initialization similar as in meta_flow case
> - to solve asymmetry issue I would propose atexit() approach, similar as it
> is implemented in ovs/lib/stopwatch.c
>
> btw, I didn't understand your first proposal. ovn_symbtab_init() function
> need to be called twice since two different
> symtab entities are initialized. Is this probably also a design issue that
> we could live with single symtab?
>
> thanks,
> damijan
>
> >
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On 2/28/20 5:40 AM, Damijan Skvarc wrote:
> Hi Mark
> 
> and thanks you made review of suggested patch.
> 
> just a few words of myself...
> I don't work professionally on this project, I started to look into this 
> code in my free hours just to satisfy my curiosity about how other 
> people work. I am following a couple of other projects just to learn 
> about different processes, tools and to keep my brains in good condition 
> while studying some others work. And if you get some things/knowledge it 
> is also kind to give back something. In case of ovs/ovn I found a nice 
> suite of automatic tests,. Unfortunately, by running them under valgrind 
> control the apparent reports become useless because of flood of memory 
> leaks. Mostly, these are small non-important leaks, however in a flood 
> of duplicated reports you can miss some serious issue. And here I found 
> my personal goal what I can give back. Unfortunately, I don't have  much 
> knowledge about networking, nor about original design. And if you don't 
> have that knowledge then from the experience I know it is better not to 
> touch anything for what you don't know why it has been designed for. So 
> every my patch is done in a way that nothing has been changed in a 
> design, just memory leak in valgrind reports disappears.

Yep, and contributions like yours are very much appreciated. If my 
criticism of your contribution came across as harsh, I apologize.

> 
> 
> On Thu, Feb 27, 2020 at 10:24 PM Mark Michelson <mmichels@redhat.com 
> <mailto:mmichels@redhat.com>> wrote:
> 
>     Hi Damijan,
> 
>     Thanks for finding the memory leak and patching it. To me,
>     ovnfield_by_name is a really strangely handled structure. There is an
>     explicit function to destroy the ovnfield_by_name structure, but its
>     creation is a side effect from ovn_symtab_init. The result is that
>     lflow.c calls ovn_symtab_init() and ovn_destroy_ovnfields(). But ofctrl
>     only calls ovn_symtab_init(). It doesn't call ovn_destroy_ovnfields()
>     because it would result in double-freeing ovnfield_by_name.
> 
> 
> yes, I noticed that asymmetry.
> 
>     This lack of symmetry shows a poor design.
> 
>     Your patch fixes the memory
>     leak, but it doesn't fix the lack of symmetry, and it still allows for
>     potential double-freeing. 
> 
>     I have a couple of suggestions for a better design:
> 
>     1) Instead of calling ovn_symbtab_init() in multiple places, call it
>     once in ovn_controller's main() function. Then when ovn_controller
>     exits, call ovn_destroy_ovnfields(). Then, pass the created symtab as a
>     parameter when it is necessary to. This works because the symtabs
>     created by lflow.c and ofctrl.c are identical, and their use of the
>     symtabs occurs in the same thread.
> 
>     2) Initialize ovnfield_by_name in a separate function from
>     ovn_symtab_init(). You can call this new ovnfield_by_name_init()
>     function in ovn-controller's main function and then call the already
>     existing ovn_destroy_ovnfields() function when ovn-controller exits.
>     This works because ovnfield_by_name does not rely on any data from the
>     created symtab. It exists completely independently.
> 
>     What do you think about this?
> 
> 
> What is strange also is that (If I see correctly) the ovnfield_by_name 
> is a hash of one single entry.
> I wouldn't use hash at all in this case. But since I don't know about 
> the design, probably some entries
> were deleted in the past and only one remained or probably there is a 
> plan some more entries will be
> added in the future. So let's assume we should not get rid of 
> ovnfield_by_name hash.
> 
> I found very similar functionality in meta_flow.c file (ovs). There 
> mf_by_name hash is more rational, it is filled with
> plenty of strings. Also initialization of this hash is more rational. It 
> is initialized only if it is needed (initialization is hidden
> in mf_from_name() and mf_from_name_len() functions and using the same 
> "pthread_once" approach as in my case). What is strange there is that 
> there is no destroy action for mf_by_name and valgrind does not report 
> any leak there. Probably test suite does not cover this functionality
That is an interesting data point. My guess is that the lack of a 
destructor for mf_by_name is an oversight. I'm not sure why valgrind 
doesn't report the leak.

> 
> I believe some more alternatives are available, however to reduce the 
> number of changes I would vote for
> - automatic and optional initialization similar as in meta_flow case
> - to solve asymmetry issue I would propose atexit() approach, similar as 
> it is implemented in ovs/lib/stopwatch.c

That's an excellent suggestion. That contains the allocation and 
destruction all within logical-fields.c without any other code having to 
know about its internals.

> btw, I didn't understand your first proposal. ovn_symbtab_init() 
> function need to be called twice since two different
> symtab entities are initialized. Is this probably also a design issue 
> that we could live with single symtab?

That's what I was thinking. Since the symtabs are identical and are 
treated as read-only (I think...), we could just have lflow and ofctrl 
share the same symtab. But I like your suggestion above better.

> 
> thanks,
> damijan
> 
>

Hi Mark

On Fri, Feb 28, 2020 at 9:50 PM Mark Michelson <mmichels@redhat.com> wrote:

> On 2/28/20 5:40 AM, Damijan Skvarc wrote:
> > Hi Mark
> >
> > and thanks you made review of suggested patch.
> >
> > just a few words of myself...
> > I don't work professionally on this project, I started to look into this
> > code in my free hours just to satisfy my curiosity about how other
> > people work. I am following a couple of other projects just to learn
> > about different processes, tools and to keep my brains in good condition
> > while studying some others work. And if you get some things/knowledge it
> > is also kind to give back something. In case of ovs/ovn I found a nice
> > suite of automatic tests,. Unfortunately, by running them under valgrind
> > control the apparent reports become useless because of flood of memory
> > leaks. Mostly, these are small non-important leaks, however in a flood
> > of duplicated reports you can miss some serious issue. And here I found
> > my personal goal what I can give back. Unfortunately, I don't have  much
> > knowledge about networking, nor about original design. And if you don't
> > have that knowledge then from the experience I know it is better not to
> > touch anything for what you don't know why it has been designed for. So
> > every my patch is done in a way that nothing has been changed in a
> > design, just memory leak in valgrind reports disappears.
>
> Yep, and contributions like yours are very much appreciated. If my
> criticism of your contribution came across as harsh, I apologize.
>
>
Sorry about making you feel you need to apologize me. It was not my
intention but just an
excuse of myself that my contribution to the project is very limited both
in time an scope.
Sorry.


> >
> >
> > On Thu, Feb 27, 2020 at 10:24 PM Mark Michelson <mmichels@redhat.com
> > <mailto:mmichels@redhat.com>> wrote:
> >
> >     Hi Damijan,
> >
> >     Thanks for finding the memory leak and patching it. To me,
> >     ovnfield_by_name is a really strangely handled structure. There is an
> >     explicit function to destroy the ovnfield_by_name structure, but its
> >     creation is a side effect from ovn_symtab_init. The result is that
> >     lflow.c calls ovn_symtab_init() and ovn_destroy_ovnfields(). But
> ofctrl
> >     only calls ovn_symtab_init(). It doesn't call ovn_destroy_ovnfields()
> >     because it would result in double-freeing ovnfield_by_name.
> >
> >
> > yes, I noticed that asymmetry.
> >
> >     This lack of symmetry shows a poor design.
> >
> >     Your patch fixes the memory
> >     leak, but it doesn't fix the lack of symmetry, and it still allows
> for
> >     potential double-freeing.
> >
> >     I have a couple of suggestions for a better design:
> >
> >     1) Instead of calling ovn_symbtab_init() in multiple places, call it
> >     once in ovn_controller's main() function. Then when ovn_controller
> >     exits, call ovn_destroy_ovnfields(). Then, pass the created symtab
> as a
> >     parameter when it is necessary to. This works because the symtabs
> >     created by lflow.c and ofctrl.c are identical, and their use of the
> >     symtabs occurs in the same thread.
> >
> >     2) Initialize ovnfield_by_name in a separate function from
> >     ovn_symtab_init(). You can call this new ovnfield_by_name_init()
> >     function in ovn-controller's main function and then call the already
> >     existing ovn_destroy_ovnfields() function when ovn-controller exits.
> >     This works because ovnfield_by_name does not rely on any data from
> the
> >     created symtab. It exists completely independently.
> >
> >     What do you think about this?
> >
> >
> > What is strange also is that (If I see correctly) the ovnfield_by_name
> > is a hash of one single entry.
> > I wouldn't use hash at all in this case. But since I don't know about
> > the design, probably some entries
> > were deleted in the past and only one remained or probably there is a
> > plan some more entries will be
> > added in the future. So let's assume we should not get rid of
> > ovnfield_by_name hash.
> >
> > I found very similar functionality in meta_flow.c file (ovs). There
> > mf_by_name hash is more rational, it is filled with
> > plenty of strings. Also initialization of this hash is more rational. It
> > is initialized only if it is needed (initialization is hidden
> > in mf_from_name() and mf_from_name_len() functions and using the same
> > "pthread_once" approach as in my case). What is strange there is that
> > there is no destroy action for mf_by_name and valgrind does not report
> > any leak there. Probably test suite does not cover this functionality
> That is an interesting data point. My guess is that the lack of a
> destructor for mf_by_name is an oversight. I'm not sure why valgrind
> doesn't report the leak.
>
> >
> > I believe some more alternatives are available, however to reduce the
> > number of changes I would vote for
> > - automatic and optional initialization similar as in meta_flow case
> > - to solve asymmetry issue I would propose atexit() approach, similar as
> > it is implemented in ovs/lib/stopwatch.c
>
> That's an excellent suggestion. That contains the allocation and
> destruction all within logical-fields.c without any other code having to
> know about its internals.
>
> > btw, I didn't understand your first proposal. ovn_symbtab_init()
> > function need to be called twice since two different
> > symtab entities are initialized. Is this probably also a design issue
> > that we could live with single symtab?
>
> That's what I was thinking. Since the symtabs are identical and are
> treated as read-only (I think...), we could just have lflow and ofctrl
> share the same symtab. But I like your suggestion above better.
>
>
I've sent v2 ( twice, because of forgotten ovn token, robot is complaining
about)

thanks again,
damijan