Re: [Xen-devel] [PATCH 0/4] xl: consolidate adhoc parsers
On Wed, Feb 17, 2016 at 10:01:37AM +, Ian Campbell wrote: > On Tue, 2016-02-16 at 23:20 +, Wei Liu wrote: > > On Tue, Feb 16, 2016 at 03:09:37PM -0700, Jim Fehlig wrote: > > > Wei Liu wrote: > > > > This patch series consolidates adhoc parsers in xl. > > > > > > Hi Wei, > > > > > > I never tested or reviewed this series after seeing Ian's comments. Did > > > you have > > > time to work on a V2? Or did I miss a V2? :-) Let me know if I can > > > help. > > > > > > > Sorry, this series fell through the crack -- it was only the work of > > one afternoon when I had some free cycles. Now I'm busy with other > > stuff and haven't had the time to pick it up again. > > > > One thing I'm not entirely happy with writing this in plain C is that > > there is subtle semantics difference from the ones generated by > > flex/bison even if they expose similar APIs. For example, the > > functions to parse disk spec won't allow you to set same attribute > > twice, while the work present in this function allows you to do > > that. It's cumbersome to implement the same functionality with open > > coding in C IMHO. > > Take care -- the xlu disk parser has a lot of additional complexity and > scaffolding to support the "legacy" xm style device specifications with > positional parameters and strange (ignored by xl) prefixes, trying to merge > that with other device types is likely to result in unwanted/unmanageable > complexity IMHO. > > I'm also not sure that switching to flex+bison for the rest is going to > solve the problems you mention above (disallowing setting something twice > etc). flex+bison will take care nicely of things like tokenising and > parsing the input (i.e. it is a nice replacement for strtok and friends), Yes, this is what I'm after as first step -- it's all very vague idea in my head at the moment. I was thinking we should use flex and bison to tokenise the input, instead of doing everything by hand. > but ultimately the keys and values need to be exposed to the user somehow > i.e. put into some data structure. > > That data structure could either be completely abstract (i.e. just provide > "lookup by key" functionality, via some sort of hash or list etc, which is > basically what the xlu cfg stuff does) or device type specific (i.e. fills > in a given struct based on a knowledge of the value keys for that struct, > which is what the disk stuff does) and it's in that code where you would > need to handle things like what to do with repeated keys. > > That is essentially just done by the C snippets in the bison rules or the > scaffolding surrounding the bison, it's not something that bison inherently > deals with. > > So ultimately you need to deal with the question of what the semantics of > setting a given key a second time are (error, take the first value, take > the last value, per-key settings etc) in C code, even if you are using > bison. That's slightly easier if you have a parser per device type, since > you can just check the specific field in the specific struct for being > unset or not before making a choice, but even in abstract code it's just a > case of checking in the list/table/hash you are constructing during parsing > for the key and reacting accordingly, but in both cases you are doing so in > C not in "bison". > Right, I do understand this. I'm heavily biased towards formalising everything in BNF -- everyone who provides a new device type should provide a parser for the configuration string in BNF, as well as some test cases in our suite. I know this is a bit unsocial so I'm trying to figure out a way if we can provide some sensible framework to do that. It's not going to be easy. And whether the return of investment is worthy of the effort is unclear. That's why I propose it to be a GSoC project. Again, perfection is the enemy of good. I think having everything done in open coding C is fine. But since we need to have similar semantics for all parsers we need to disallow setting same value twice, the boilerplate for that is just tedious to write. Wei. > Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 0/4] xl: consolidate adhoc parsers
On Tue, 2016-02-16 at 23:20 +, Wei Liu wrote: > On Tue, Feb 16, 2016 at 03:09:37PM -0700, Jim Fehlig wrote: > > Wei Liu wrote: > > > This patch series consolidates adhoc parsers in xl. > > > > Hi Wei, > > > > I never tested or reviewed this series after seeing Ian's comments. Did > > you have > > time to work on a V2? Or did I miss a V2? :-) Let me know if I can > > help. > > > > Sorry, this series fell through the crack -- it was only the work of > one afternoon when I had some free cycles. Now I'm busy with other > stuff and haven't had the time to pick it up again. > > One thing I'm not entirely happy with writing this in plain C is that > there is subtle semantics difference from the ones generated by > flex/bison even if they expose similar APIs. For example, the > functions to parse disk spec won't allow you to set same attribute > twice, while the work present in this function allows you to do > that. It's cumbersome to implement the same functionality with open > coding in C IMHO. Take care -- the xlu disk parser has a lot of additional complexity and scaffolding to support the "legacy" xm style device specifications with positional parameters and strange (ignored by xl) prefixes, trying to merge that with other device types is likely to result in unwanted/unmanageable complexity IMHO. I'm also not sure that switching to flex+bison for the rest is going to solve the problems you mention above (disallowing setting something twice etc). flex+bison will take care nicely of things like tokenising and parsing the input (i.e. it is a nice replacement for strtok and friends), but ultimately the keys and values need to be exposed to the user somehow i.e. put into some data structure. That data structure could either be completely abstract (i.e. just provide "lookup by key" functionality, via some sort of hash or list etc, which is basically what the xlu cfg stuff does) or device type specific (i.e. fills in a given struct based on a knowledge of the value keys for that struct, which is what the disk stuff does) and it's in that code where you would need to handle things like what to do with repeated keys. That is essentially just done by the C snippets in the bison rules or the scaffolding surrounding the bison, it's not something that bison inherently deals with. So ultimately you need to deal with the question of what the semantics of setting a given key a second time are (error, take the first value, take the last value, per-key settings etc) in C code, even if you are using bison. That's slightly easier if you have a parser per device type, since you can just check the specific field in the specific struct for being unset or not before making a choice, but even in abstract code it's just a case of checking in the list/table/hash you are constructing during parsing for the key and reacting accordingly, but in both cases you are doing so in C not in "bison". Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 0/4] xl: consolidate adhoc parsers
On Tue, Feb 16, 2016 at 03:09:37PM -0700, Jim Fehlig wrote: > Wei Liu wrote: > > This patch series consolidates adhoc parsers in xl. > > Hi Wei, > > I never tested or reviewed this series after seeing Ian's comments. Did you > have > time to work on a V2? Or did I miss a V2? :-) Let me know if I can help. > Sorry, this series fell through the crack -- it was only the work of one afternoon when I had some free cycles. Now I'm busy with other stuff and haven't had the time to pick it up again. One thing I'm not entirely happy with writing this in plain C is that there is subtle semantics difference from the ones generated by flex/bison even if they expose similar APIs. For example, the functions to parse disk spec won't allow you to set same attribute twice, while the work present in this function allows you to do that. It's cumbersome to implement the same functionality with open coding in C IMHO. To get to the point where I feel happy requires quite a bit of work, so I put "consolidating xl ad hoc parser" (a super set of this series) to this year's GSoC this morning. Basically I want to consolidate ad hoc parser, let them have similar semantics if possible, and then provide a test suite so that it won't be broken by mistake. That being said, perfection is the enemy of good. If you think this is important, I will try to find some time to refresh this series; but I don't have time for the test suite yet. You're welcome to take over this series if you have time. Wei. (Please forgive my errors in grammar etc, it's a bit late at night here...) > Regards, > Jim ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 0/4] xl: consolidate adhoc parsers
Wei Liu wrote: > This patch series consolidates adhoc parsers in xl. Hi Wei, I never tested or reviewed this series after seeing Ian's comments. Did you have time to work on a V2? Or did I miss a V2? :-) Let me know if I can help. Regards, Jim ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 0/4] xl: consolidate adhoc parsers
On Fri, 2016-01-22 at 11:50 +, Wei Liu wrote: > This patch series consolidates adhoc parsers in xl. > > There are currently 4 types of devices: > > 1. block > 2. netowrk > 3. vtpm > 4. pci > > that support hotplug as well as being specified in config file. > > Block and pci devices are fine because they use libxlu to parse > configuration strings. > > Network and vtpms config parsers are not in a very good state, which > need to be consolidated. > > Note that there is code repetition in the newly introduced parser > code. We either need to have very long macro definition or code > repetition. I chose the latter. Let me know your opinion. You could make it take function pointers by using void * to point to the objects. There's a (small) risk of mismatching the point and the callback, but not significant enough to worry about IMHO. That said, when I mentioned helpers originally I was imagining they would parse their input into a data structure containing (in some form or other) a list of key value pairs (taking into account the behaviour desired on duplication of keys etc) which the callers would use opaquely up front and then iterate over the results in some way (either a foreach helper macro, or first() and next() type helpers etc). Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 0/4] xl: consolidate adhoc parsers
This patch series consolidates adhoc parsers in xl. There are currently 4 types of devices: 1. block 2. netowrk 3. vtpm 4. pci that support hotplug as well as being specified in config file. Block and pci devices are fine because they use libxlu to parse configuration strings. Network and vtpms config parsers are not in a very good state, which need to be consolidated. Note that there is code repetition in the newly introduced parser code. We either need to have very long macro definition or code repetition. I chose the latter. Let me know your opinion. Wei. Wei Liu (4): xl: remove unused macros xl: wrap long lines where possible xl: rework vif config parsing code xl: rework vtpm config parsing code tools/libxl/xl_cmdimpl.c | 312 +++ 1 file changed, 207 insertions(+), 105 deletions(-) -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
