From: Ian Jackson <Ian.Jackson@eu.citrix.com>
To: "Chen, Tiejun" <tiejun.chen@intel.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
Wei Liu <wei.liu2@citrix.com>,
Ian Campbell <ian.campbell@citrix.com>,
xen-devel@lists.xen.org
Subject: Re: [v7][PATCH 16/16] tools: parse to enable new rdm policy parameters
Date: Fri, 10 Jul 2015 11:23:29 +0100 [thread overview]
Message-ID: <21919.40225.618413.570220@mariner.uk.xensource.com> (raw)
In-Reply-To: <559F60AB.2060402@intel.com>
Chen, Tiejun writes ("Re: [v7][PATCH 16/16] tools: parse to enable new rdm policy parameters"):
> > The first issue (which would really be relevant to the documentation
> > patch) is that the documentation is in a separate commit. There are
> > sometimes valid reasons for doing this. I'm not sure if they apply,
>
> Wei suggested we should organize/spit all patches according to libxl,
> libxc, xc and xl.
Yes. I don't want to put you in the middle of a
disagreement/misunderstanding between Wei and me. This is in any case
a fairly minor issue.
> In this patch head description, maybe I can change something like this
>
> This patch parses to enable user configurable parameters to specify
> RDM resource and according policies which are defined previously,
That would be good, yes, thanks.
> >> + }else if ( !strcmp(optkey, "rdm_policy") ) {
> >> + if ( !strcmp(tok, "strict") ) {
> >> + pcidev->rdm_policy = LIBXL_RDM_RESERVE_POLICY_STRICT;
> >> + } else if ( !strcmp(tok, "relaxed") ) {
> >> + pcidev->rdm_policy = LIBXL_RDM_RESERVE_POLICY_RELAXED;
> >> + } else {
> >> + XLU__PCI_ERR(cfg, "%s is not an valid PCI RDM property"
> >> + " policy: 'strict' or 'relaxed'.",
> >> + tok);
> >> + goto parse_error;
> >> + }
> >
> > This section has coding style (whitespace) problems and long lines.
> > If you need to respin, please fix them.
>
> Are you saying this?
>
> } else if ( -> }else if (
> } else { -> }else {
Also spurious spaces inside brackets. Please see CODING_STYLE.
> Additionally I don't found which line is over 80 characters.
Hmm, I see that the longest line is exactly 80.
Of course 80 characters, plus a `+' from a patch, and `>' for email
quoting, makes more than 80: so it has wrap damage on my screen. You
are right that this conforms to the wording in the style, which says
`75-80'. So I won't insist on this change (unless I manage to
persuade my comaintainers to strenghten this restriction in the style
guide).
> >> + for (tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) {
> >> + switch(state) {
> >> + case STATE_TYPE:
> >
> > This code is extremely repetitive.
>
> I just refer to xlu_pci_parse_bdf()
Yes. I'm afraid that xlu_pci_parse_bdf is a poor example.
> > Really I would prefer that this parsing was done with a miniature flex
> > parser, rather than ad-hoc pointer arithmetic and use of strtok.
>
> Sorry, could you show this explicitly?
Something like what was done for disk devices. See libxlu_disk_l.l
for an example. In this case your code would be a lot less
complicated than what you see there.
After the codefreeze I would probably have some time to write it for
you. (I think that would be valuable because libxlu_disk_l.l is a
very complicated example, and I want be able to point future
submitters at something simpler.)
Ian.
next prev parent reply other threads:[~2015-07-10 10:23 UTC|newest]
Thread overview: 119+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-09 5:33 [v7][PATCH 00/16] Fix RMRR Tiejun Chen
2015-07-09 5:33 ` [v7][PATCH 01/16] xen: introduce XENMEM_reserved_device_memory_map Tiejun Chen
2015-07-09 5:33 ` [v7][PATCH 02/16] xen/vtd: create RMRR mapping Tiejun Chen
2015-07-09 5:33 ` [v7][PATCH 03/16] xen/passthrough: extend hypercall to support rdm reservation policy Tiejun Chen
2015-07-10 13:26 ` George Dunlap
2015-07-10 15:01 ` Jan Beulich
2015-07-10 15:07 ` George Dunlap
2015-07-13 6:37 ` Chen, Tiejun
2015-07-13 5:57 ` Chen, Tiejun
2015-07-13 6:47 ` Chen, Tiejun
2015-07-13 8:57 ` Jan Beulich
2015-07-14 10:46 ` George Dunlap
2015-07-14 10:53 ` Chen, Tiejun
2015-07-14 11:30 ` George Dunlap
2015-07-14 11:45 ` Jan Beulich
2015-07-14 13:25 ` George Dunlap
2015-07-09 5:33 ` [v7][PATCH 04/16] xen: enable XENMEM_memory_map in hvm Tiejun Chen
2015-07-09 5:33 ` [v7][PATCH 05/16] hvmloader: get guest memory map into memory_map[] Tiejun Chen
2015-07-10 13:49 ` George Dunlap
2015-07-13 7:03 ` Chen, Tiejun
2015-07-09 5:33 ` [v7][PATCH 06/16] hvmloader/pci: skip reserved ranges Tiejun Chen
2015-07-13 13:12 ` Jan Beulich
2015-07-14 6:39 ` Chen, Tiejun
2015-07-14 9:27 ` Jan Beulich
2015-07-14 10:54 ` Chen, Tiejun
2015-07-14 11:50 ` Jan Beulich
2015-07-15 0:55 ` Chen, Tiejun
2015-07-15 4:27 ` Chen, Tiejun
2015-07-15 8:34 ` Jan Beulich
2015-07-15 8:59 ` Chen, Tiejun
2015-07-15 9:10 ` Chen, Tiejun
2015-07-15 9:27 ` Jan Beulich
2015-07-15 10:34 ` Chen, Tiejun
2015-07-15 11:25 ` Jan Beulich
2015-07-15 11:34 ` Chen, Tiejun
2015-07-15 13:56 ` George Dunlap
2015-07-15 16:14 ` George Dunlap
2015-07-16 2:05 ` Chen, Tiejun
2015-07-16 9:40 ` George Dunlap
2015-07-16 10:01 ` Chen, Tiejun
2015-07-15 11:05 ` George Dunlap
2015-07-15 11:20 ` Chen, Tiejun
2015-07-15 12:43 ` George Dunlap
2015-07-15 13:23 ` Chen, Tiejun
2015-07-15 11:24 ` Jan Beulich
2015-07-15 11:38 ` George Dunlap
2015-07-15 11:27 ` Jan Beulich
2015-07-15 11:40 ` Chen, Tiejun
2015-07-15 8:32 ` Jan Beulich
2015-07-15 9:04 ` Chen, Tiejun
2015-07-15 12:57 ` Wei Liu
2015-07-15 13:40 ` George Dunlap
2015-07-15 14:00 ` Jan Beulich
2015-07-15 15:19 ` George Dunlap
2015-07-09 5:33 ` [v7][PATCH 07/16] hvmloader/e820: construct guest e820 table Tiejun Chen
2015-07-13 13:35 ` Jan Beulich
2015-07-14 5:22 ` Chen, Tiejun
2015-07-14 9:32 ` Jan Beulich
2015-07-14 10:22 ` Chen, Tiejun
2015-07-14 10:48 ` Jan Beulich
2015-07-15 16:00 ` George Dunlap
2015-07-16 1:58 ` Chen, Tiejun
2015-07-16 9:41 ` George Dunlap
2015-07-09 5:33 ` [v7][PATCH 08/16] tools/libxc: Expose new hypercall xc_reserved_device_memory_map Tiejun Chen
2015-07-09 5:34 ` [v7][PATCH 09/16] tools: extend xc_assign_device() to support rdm reservation policy Tiejun Chen
2015-07-09 5:34 ` [v7][PATCH 10/16] tools: introduce some new parameters to set rdm policy Tiejun Chen
2015-07-09 9:20 ` Wei Liu
2015-07-09 9:44 ` Chen, Tiejun
2015-07-09 10:37 ` Ian Jackson
2015-07-09 10:53 ` Chen, Tiejun
2015-07-09 18:02 ` Ian Jackson
2015-07-10 0:46 ` Chen, Tiejun
2015-07-09 5:34 ` [v7][PATCH 11/16] tools/libxl: detect and avoid conflicts with RDM Tiejun Chen
2015-07-09 9:11 ` Wei Liu
2015-07-09 9:41 ` Chen, Tiejun
2015-07-09 18:14 ` Ian Jackson
2015-07-10 3:19 ` Chen, Tiejun
2015-07-10 10:14 ` Ian Jackson
2015-07-13 9:19 ` Chen, Tiejun
2015-07-09 5:34 ` [v7][PATCH 12/16] tools: introduce a new parameter to set a predefined rdm boundary Tiejun Chen
2015-07-09 18:14 ` Ian Jackson
2015-07-09 5:34 ` [v7][PATCH 13/16] libxl: construct e820 map with RDM information for HVM guest Tiejun Chen
2015-07-09 18:17 ` Ian Jackson
2015-07-10 5:40 ` Chen, Tiejun
2015-07-10 9:18 ` Ian Campbell
2015-07-13 9:47 ` Chen, Tiejun
2015-07-13 10:15 ` Ian Campbell
2015-07-14 5:44 ` Chen, Tiejun
2015-07-14 7:42 ` Ian Campbell
2015-07-14 8:03 ` Chen, Tiejun
2015-07-10 10:15 ` Ian Jackson
2015-07-09 5:34 ` [v7][PATCH 14/16] xen/vtd: enable USB device assignment Tiejun Chen
2015-07-09 5:34 ` [v7][PATCH 15/16] xen/vtd: prevent from assign the device with shared rmrr Tiejun Chen
2015-07-13 13:41 ` Jan Beulich
2015-07-14 1:42 ` Chen, Tiejun
2015-07-14 9:19 ` Jan Beulich
2015-07-09 5:34 ` [v7][PATCH 16/16] tools: parse to enable new rdm policy parameters Tiejun Chen
2015-07-09 18:23 ` Ian Jackson
2015-07-10 6:05 ` Chen, Tiejun
2015-07-10 10:23 ` Ian Jackson [this message]
2015-07-13 9:31 ` Chen, Tiejun
2015-07-13 9:40 ` Ian Campbell
2015-07-13 9:55 ` Chen, Tiejun
2015-07-13 10:17 ` Ian Campbell
2015-07-13 17:08 ` Ian Jackson
2015-07-14 1:29 ` Chen, Tiejun
2015-07-10 14:50 ` [v7][PATCH 00/16] Fix RMRR George Dunlap
2015-07-10 14:56 ` Jan Beulich
2015-07-16 7:55 ` Jan Beulich
2015-07-16 8:03 ` Chen, Tiejun
2015-07-16 8:08 ` Jan Beulich
2015-07-16 8:13 ` Chen, Tiejun
2015-07-16 8:26 ` Jan Beulich
2015-07-16 9:27 ` George Dunlap
2015-07-16 9:44 ` Jan Beulich
2015-07-16 9:59 ` George Dunlap
2015-07-16 8:30 ` Ian Campbell
2015-07-16 8:46 ` Wei Liu
2015-07-16 9:45 ` Lars Kurth
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=21919.40225.618413.570220@mariner.uk.xensource.com \
--to=ian.jackson@eu.citrix.com \
--cc=ian.campbell@citrix.com \
--cc=stefano.stabellini@eu.citrix.com \
--cc=tiejun.chen@intel.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).