On Mon, Nov 17, 2014 at 3:11 PM, Rainer Gerhards <[email protected]> wrote:
> 2014-11-17 10:35 GMT+01:00 singh.janmejay <[email protected]>: > > > So for users that do not want tree merging, is 'reset' the right answer? > > > > Of the form: > > > > reset $.foo = $!bar; > > > > As opposed to set, this will simply remove whatever value is present at > foo > > and set the given value in. > > > > > I guess so. But quite honestly, I would really need to read the code and > commits in detail. That past couple of weeks I've been building VMs and > working on testbench automation, so I really do not have this on my radar > right now. So unfortunately it's nothing I can answer right out of my head. > I also admit that I do not want to leave the testbench area right now, as I > have invested so much time into it and breaking away from it can waste a > lot of this time. > > The general story is this: we needed to implement variables in a tree-like > manner. There have been a couple of choices at those times (among them was > a part of ding-libs, various json libs, "do it yourself" implementation). > For simplicity, we settled on json (and I think switched libs to json-c a > bit later). If we now reset containers, each set affect all other variables > in that container, which would not make much sense. In that light, an > explicit reset sounds like the right thing to do. But again, I would really > need some more time to look at this and the implications. The foreach patch needs a 'reset' kind of control to make the loop implementation useful. Sounds like we can go with 'reset' for now (so I can complete the patch) and once you have had time to re-look at the merge/leaf-node special behaviour, we can either let 'reset' live(and keep set unchanged), or kill it after making 'set' identical to it (if it doesn't have any undesirable effects). So shall I go ahead and implement 'reset'? > I am also not > sure if the current tests capture all side-effects possible ... simply > because I did not think about this when I implemented and wrote the tests > (did sombody mention that it is a bad thing if a single person implements, > writes tests and documents? ;)). > > Hope that helps at least a bit. > > Rainer > > > > On Mon, Nov 17, 2014 at 2:58 PM, Rainer Gerhards < > [email protected] > > > > > wrote: > > > > > I need to review this in detail. IIRC, this is required by some > modules. > > > That "set" can manipulate trees is more or less a side-effect of the > > > variable implementation via json-c. Replacing json-c with something > that > > > offers more performance is on the todo list for quite a while. > > > > > > Rainer > > > > > > 2014-11-17 10:23 GMT+01:00 singh.janmejay <[email protected]>: > > > > > > > Bump. > > > > > > > > On Thu, Nov 13, 2014 at 12:13 PM, singh.janmejay < > > > [email protected] > > > > > > > > > wrote: > > > > > > > > > Also, tests seem to pass with the change I mentioned above (set > > > replacing > > > > > the contents of field regardless of it being object, leaf or null). > > > > > > > > > > > > > > > > > > > > ============================================================================ > > > > > Testsuite summary for rsyslog 8.5.0 > > > > > > > > > > > > > > > ============================================================================ > > > > > # TOTAL: 125 # PASS: 120 # SKIP: 5 # XFAIL: 0 # FAIL: 0 # XPASS: 0 > # > > > > ERROR: > > > > > 0 > > > > > > > > > > > > > > > ============================================================================ > > > > > > > > > > > > > > > On Thu, Nov 13, 2014 at 12:11 PM, singh.janmejay < > > > > [email protected] > > > > > > wrote: > > > > > > > > > >> Removing all the cases and replacing everything with just: > > > > >> > > > > >> json_object_object_add(parent, (char*)leaf, json); > > > > >> > > > > >> Changes the semantics to always replace the value, regardless of > it > > > > being > > > > >> object, leaf or null. > > > > >> > > > > >> From the pov of set x = y, it seems like the right thing to do. > > > > >> > > > > >> But im sure the existing implementation is the way it is for a > > reason. > > > > >> > > > > >> In case its just a bug, should we go ahead with this replacement? > > (all > > > > >> cases removed and just one simple object_add call, no dereference > of > > > old > > > > >> value required either). > > > > >> > > > > >> In case its not a bug, what about having another statement > (reset?) > > > for > > > > >> this purpose? > > > > >> > > > > >> So, if user wants objects to be merged and object not be be > replaced > > > > with > > > > >> leaf etc, they can use 'set $.foo = $.bar;' but if they want > > > > no-conditions > > > > >> replace-whatever semantics, they can use 'reset $.foo = $.bar;'. > > > > >> > > > > >> Ideally, may be we should call set something else, merge? but that > > may > > > > be > > > > >> bad from backward compatibility pov. > > > > >> > > > > >> On Thu, Nov 13, 2014 at 11:07 AM, singh.janmejay < > > > > >> [email protected]> wrote: > > > > >> > > > > >>> Hi Rainer, > > > > >>> > > > > >>> Im talking about msgAddJSON. Quickly glanced through your > > > > >>> commit(71a5122fa), but it doesn't seem to talk much about it. Why > > do > > > we > > > > >>> disallow replacing an object with a non-object value? > > > > >>> > > > > >>> Also, not really the same issue, but another clarification in the > > > same > > > > >>> area of code. Why do we merge objects when user has called 'set'? > > > > shouldn't > > > > >>> we replace old json_object with new one? > > > > >>> > > > > >>> -- > > > > >>> Regards, > > > > >>> Janmejay > > > > >>> http://codehunk.wordpress.com > > > > >>> > > > > >> > > > > >> > > > > >> > > > > >> -- > > > > >> Regards, > > > > >> Janmejay > > > > >> http://codehunk.wordpress.com > > > > >> > > > > > > > > > > > > > > > > > > > > -- > > > > > Regards, > > > > > Janmejay > > > > > http://codehunk.wordpress.com > > > > > > > > > > > > > > > > > > > > > -- > > > > Regards, > > > > Janmejay > > > > http://codehunk.wordpress.com > > > > _______________________________________________ > > > > rsyslog mailing list > > > > http://lists.adiscon.net/mailman/listinfo/rsyslog > > > > http://www.rsyslog.com/professional-services/ > > > > What's up with rsyslog? Follow https://twitter.com/rgerhards > > > > NOTE WELL: This is a PUBLIC mailing list, posts are ARCHIVED by a > > myriad > > > > of sites beyond our control. PLEASE UNSUBSCRIBE and DO NOT POST if > you > > > > DON'T LIKE THAT. > > > > > > > _______________________________________________ > > > rsyslog mailing list > > > http://lists.adiscon.net/mailman/listinfo/rsyslog > > > http://www.rsyslog.com/professional-services/ > > > What's up with rsyslog? Follow https://twitter.com/rgerhards > > > NOTE WELL: This is a PUBLIC mailing list, posts are ARCHIVED by a > myriad > > > of sites beyond our control. PLEASE UNSUBSCRIBE and DO NOT POST if you > > > DON'T LIKE THAT. > > > > > > > > > > > -- > > Regards, > > Janmejay > > http://codehunk.wordpress.com > > _______________________________________________ > > rsyslog mailing list > > http://lists.adiscon.net/mailman/listinfo/rsyslog > > http://www.rsyslog.com/professional-services/ > > What's up with rsyslog? Follow https://twitter.com/rgerhards > > NOTE WELL: This is a PUBLIC mailing list, posts are ARCHIVED by a myriad > > of sites beyond our control. PLEASE UNSUBSCRIBE and DO NOT POST if you > > DON'T LIKE THAT. > > > _______________________________________________ > rsyslog mailing list > http://lists.adiscon.net/mailman/listinfo/rsyslog > http://www.rsyslog.com/professional-services/ > What's up with rsyslog? Follow https://twitter.com/rgerhards > NOTE WELL: This is a PUBLIC mailing list, posts are ARCHIVED by a myriad > of sites beyond our control. PLEASE UNSUBSCRIBE and DO NOT POST if you > DON'T LIKE THAT. > -- Regards, Janmejay http://codehunk.wordpress.com _______________________________________________ rsyslog mailing list http://lists.adiscon.net/mailman/listinfo/rsyslog http://www.rsyslog.com/professional-services/ What's up with rsyslog? Follow https://twitter.com/rgerhards NOTE WELL: This is a PUBLIC mailing list, posts are ARCHIVED by a myriad of sites beyond our control. PLEASE UNSUBSCRIBE and DO NOT POST if you DON'T LIKE THAT.
