- flashrom-gerrit - mail.coreboot.org

Change in flashrom[refs/meta/config]: Modify access rules
by Stefan Tauner (Code Review) 16 Aug '17

16 Aug '17

Stefan Tauner has uploaded this change for review. ( https://review.coreboot.org/21056 Change subject: Modify access rules ...................................................................... Modify access rules Change-Id: I5976f78f53328c7406133e811a1da87fa037ec0e --- M groups M project.config 2 files changed, 5 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/56/21056/1 diff --git a/groups b/groups index aa8425b..3caeb32 100644 --- a/groups +++ b/groups @@ -2,3 +2,4 @@ # db95ce11b379445ac8c5806ea0b61195555b338d flashrom developers user:dhendrix user/David Hendricks (dhendrix) +user:stefanct user/Stefan Tauner (stefanct) diff --git a/project.config b/project.config index f8a6395..88bd0b0 100644 --- a/project.config +++ b/project.config @@ -9,6 +9,7 @@ submit = group flashrom developers [access "refs/heads/staging"] submit = group flashrom developers + exclusiveGroupPermissions = submit [access "refs/tags/*"] push = +force group flashrom developers @@ -31,4 +32,7 @@ filter = -is:draft [access "refs/heads/master"] create = group user/David Hendricks (dhendrix) + create = group user/Stefan Tauner (stefanct) delete = group user/David Hendricks (dhendrix) + delete = group user/Stefan Tauner (stefanct) + exclusiveGroupPermissions = create delete -- To view, visit https://review.coreboot.org/21056 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: refs/meta/config Gerrit-MessageType: newchange Gerrit-Change-Id: I5976f78f53328c7406133e811a1da87fa037ec0e Gerrit-Change-Number: 21056 Gerrit-PatchSet: 1 Gerrit-Owner: Stefan Tauner <stefan.tauner(a)gmx.at>

1 0

Change in flashrom[staging]: Include <sys/types.h> wherever ssize_t is used
by Nico Huber (Code Review) 16 Aug '17

16 Aug '17

Nico Huber has posted comments on this change. ( https://review.coreboot.org/21015 ) Change subject: Include <sys/types.h> wherever ssize_t is used ...................................................................... Patch Set 3: > +2 but all i can do here is +1 :P Should be fixed :) -- To view, visit https://review.coreboot.org/21015 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Gerrit-Change-Number: 21015 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 16 Aug 2017 17:02:37 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Include <sys/types.h> wherever ssize_t is used
by Nico Huber (Code Review) 16 Aug '17

16 Aug '17

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/21015 ) Change subject: Include <sys/types.h> wherever ssize_t is used ...................................................................... Include <sys/types.h> wherever ssize_t is used `ssize_t` is a POSIX type (cf. IEEE Std 1003.1). Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reported-by: Urja Rannikko <urjaman(a)gmail.com> Reviewed-on: https://review.coreboot.org/21015 Reviewed-by: David Hendricks <david.hendricks(a)gmail.com> Reviewed-by: Urja Rannikko <urjaman(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M dediprog.c M ich_descriptors.c M ich_descriptors.h 3 files changed, 3 insertions(+), 0 deletions(-) Approvals: David Hendricks: Looks good to me, approved Urja Rannikko: Looks good to me, but someone else must approve build bot (Jenkins): Verified diff --git a/dediprog.c b/dediprog.c index 6f82772..2f5b441 100644 --- a/dediprog.c +++ b/dediprog.c @@ -21,6 +21,7 @@ #include "platform.h" +#include <sys/types.h> #include <stdlib.h> #include <stdio.h> #include <string.h> diff --git a/ich_descriptors.c b/ich_descriptors.c index a2f8edf..7916152 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -34,6 +34,7 @@ #define UPPER_MAP_OFFSET (4096 - 256 - 4) #define getVTBA(flumap) (((flumap)->FLUMAP1 << 4) & 0x00000ff0) +#include <sys/types.h> #include <string.h> #include "flash.h" /* for msg_* */ #include "programmer.h" diff --git a/ich_descriptors.h b/ich_descriptors.h index 574bc5f..4ddc710 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -22,6 +22,7 @@ #ifndef __ICH_DESCRIPTORS_H__ #define __ICH_DESCRIPTORS_H__ 1 +#include <sys/types.h> #include <stdint.h> #include "programmer.h" /* for enum ich_chipset */ -- To view, visit https://review.coreboot.org/21015 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: merged Gerrit-Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Gerrit-Change-Number: 21015 Gerrit-PatchSet: 3 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

Change in flashrom[staging]: Include <sys/types.h> wherever ssize_t is used
by build bot (Jenkins) (Code Review) 16 Aug '17

16 Aug '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/21015 ) Change subject: Include <sys/types.h> wherever ssize_t is used ...................................................................... Patch Set 2: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/509/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/469/ : SUCCESS -- To view, visit https://review.coreboot.org/21015 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Gerrit-Change-Number: 21015 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 16 Aug 2017 12:32:14 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Include <sys/types.h> wherever ssize_t is used
by Urja Rannikko (Code Review) 16 Aug '17

16 Aug '17

Urja Rannikko has posted comments on this change. ( https://review.coreboot.org/21015 ) Change subject: Include <sys/types.h> wherever ssize_t is used ...................................................................... Patch Set 1: Code-Review+1 +2 but all i can do here is +1 :P -- To view, visit https://review.coreboot.org/21015 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Gerrit-Change-Number: 21015 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 16 Aug 2017 05:34:18 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Include <sys/types.h> wherever ssize_t is used
by David Hendricks (Code Review) 16 Aug '17

16 Aug '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/21015 ) Change subject: Include <sys/types.h> wherever ssize_t is used ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/21015 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Gerrit-Change-Number: 21015 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Urja Rannikko <urjaman(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Wed, 16 Aug 2017 00:39:07 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Convert flashrom to git
by Stefan Tauner (Code Review) 16 Aug '17

16 Aug '17

Stefan Tauner has posted comments on this change. ( https://review.coreboot.org/19206 ) Change subject: Convert flashrom to git ...................................................................... Patch Set 11: (15 comments) FWIW, these are my replies - mostly to Nico's excellent review - that also try to show rationales where my current patch diverges from what eventually landed in staging as of now. I still need to figure out how to integrate gerrit (and its hooks) into the scheme and will publish it then (with the intention to make it the first stable commit after 0.9.9). https://review.coreboot.org/#/c/19206/1//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/19206/1//COMMIT_MSG@28 PS1, Line 28: here are a few major differences: : - This uses coreboot's commit-msg hook which includes support for : generating and appending Change-Id. : - djgpp-dos target removal is moved to a > This seems way over-complicated with no visible use case. It also makes the Of course it is over-complicated - it's what Carl-Daniel and I came up with when I convinced him to switch to git by fulfilling his requirements - namely having version strings with some kind of monotonicity that can easily be referred to by humans. Programmatically it is trivial to just use the git hash and derive some other string like that of git describe if that's needed by anybody. I don't see why this would be needed though? The "visible use case" of the implemented scheme is simply... any log file posted that any developer needs to read? Apart from the additional complexity of the get_revision script I don't really see any downsides. I put some substantial effort into making the high-quality transition to git possible, which includes this borderline-insane scheme to create somewhat monotonic version strings out of git without too much hassle for anybody. Of course I would like to keep it and there need to be better arguments then "meh this looks too complicated and I don't need it" to convince me otherwise. https://review.coreboot.org/#/c/19206/1//COMMIT_MSG@63 PS1, Line 63: : > Honestly, who cares? If somebody gets on the mailing list and presents this It still gives a relatively good indication of what is contained in the code for those familiar with the commit history *without* checking a VCS. https://review.coreboot.org/#/c/19206/1/Makefile File Makefile: https://review.coreboot.org/#/c/19206/1/Makefile@1357 PS1, Line 1357: > This overwrites the default system-management heading (center of Basically, because I think it is a good idea to give the user the version on the top instead of the rather useless manual section name. :) And I would have never thought anybody would review this change in such detail so I simply slipped it in. If there is no objection I will simply note it in the commit message of the stable branch... https://review.coreboot.org/#/c/19206/1/Makefile@1369 PS1, Line 1369: @ { $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS) .featuretest.c -o .featuretest$(EXEC_SUFFIX) >&2 && \ : ( echo "found."; echo "UTSNAME := yes" >> .features.tmp ) || \ : ( echo "not found."; echo "UTSNAME := no" >> .features.tmp ) } 2>>$(BUILD_DETAILS_FILE) | tee -a $(BUILD_DETAILS_FILE) : @printf "Checking for clock_gettime support... " | tee -a The makefile does not modify itself. It generates a modified version of itself in the export directory. :P But I see your point. My implementation for your idea is slightly different to David's but essentially also creating and using a Makefile.version file. https://review.coreboot.org/#/c/19206/1/Makefile@1377 PS1, Line 1377: t found."; echo "CLOCK > I think 'x;n;n;/^set$$/{b};x;p' would be more clear. Is it? 'b' is documented in POSIX and sed's info pages as unconditional. In the case above this is kind of moot because there is a condition that guards the unconditional branch: the address using a regex to match the respective lines. Took me a while to grasp because I never really used addresses so far. When knowing this, the construct is nicer, true. I'll change it and also improve the comments. One question: why the '{}' around 'b'? https://review.coreboot.org/#/c/19206/1/Makefile@1381 PS1, Line 1381: > ...could be simply fixed by adding a dummy after the command like Good catch! Thank you. https://review.coreboot.org/#/c/19206/4/Makefile File Makefile: https://review.coreboot.org/#/c/19206/4/Makefile@a1374 PS4, Line 1374: > Why didn't I bikeshed this for being totally unrelated? It should have been removed a very long time ago, and like other minor things I did not expect anybody to notice its removal in a review at all... https://review.coreboot.org/#/c/19206/1/util/getrevision.sh File util/getrevision.sh: https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@44 PS1, Line 44: } : : git_is_file_tracked() { > Do we break lines only at full stops now? This looks like a single No, we break at the latest at a 112 character limit because that's what was agreed upon when the matter of line breaking was last discussed on the mailing list in 2012. In this case I broke it early but I cannot say why and this is indeed a bug. Possibly because breaking after every full stop is the style used when I write text documents (Latex etc). https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@229 PS1, Line 229: query_ > So we never synchronize in the upstream-repo-registered case but The idea behind this is simply to optimize performance for the common case. If the user uses upstream repos the code assumes that the user updates them in case some commits are merged upstream. It is rather improbable that a user continuously implements changes on top of a single (aging) upstream version (unless upstream is not updated for years but that's another story ;) without fetching any updates. I have added this explanation to the source comment (the user output explains the updates of the upcache already and i dont want to complicate it ever further). https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@257 PS1, Line 257: : : : : : : > Nit, looks like `staging` would override `stable` here even if they AFAICT the order is determined by that in 'upstream_branches' but I have not re-tested this now. Why do you think otherwise? https://review.coreboot.org/#/c/19206/1/util/getrevision.sh@269 PS1, Line 269: > always true on this path? No, there might be upstream branches that are neither stable nor staging (e.g. a release branch such as 0.9.9 for bug fixes). There is also an example in the commit message. https://review.coreboot.org/#/c/19206/1/util/git-hooks/install.sh File util/git-hooks/install.sh: https://review.coreboot.org/#/c/19206/1/util/git-hooks/install.sh@17 PS1, Line 17: $(git > This assumption looks rather odd given the `git rev-parse` above. No, IIRC, the idea is to make these relative links because else they get invalid when someone moves the checked out directory. https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push File util/git-hooks/pre-push: https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push@1 PS1, Line 1: #!/bin/sh > Since we probably want to avoid bashisms in general for git hooks, I got ri What David(?) said... https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push@53 PS1, Line 53: ommit $local_sha i > I reworded this to try and make it less confusing. LMK what you think. Much clearer, thank you. https://review.coreboot.org/#/c/19206/1/util/git-hooks/pre-push@57 PS1, Line 57: : # Make _really_ sure we do not rewrite precious history : for lbranch in $precious_branches ; do : if [ "$remote_ref" = "refs/heads/$lbranch" ]; then : nonreachable=$(git rev-list $remote_sha ^$local_sha | head -1) : if [ -n "$nonreachable" ]; then : echo "Only fast-forward pushes are allowed on $lbranch." >&2 : echo "$nonreachable is not included in $remote_sha while pushing to $remote_ref" >&2 : exit 1 : fi : fi > Isn't this configurable in the upstream repo? The patch was written when I still hoped we could avoid Gerrit... and plain git (as used on flashrom.org/git) does not support that. I have not thought about how to deal with the two repositories in the hooks yet... just catching up with the changes to this patch atm. -- To view, visit https://review.coreboot.org/19206 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I64eef21982cac0a0a7419bcd2c8a936672ae9cb2 Gerrit-Change-Number: 19206 Gerrit-PatchSet: 11 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Martin Roth <martinroth(a)google.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Philippe Mathieu-Daudé <f4bug(a)amsat.org> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Reviewer: david.hendricks(a)gmail.com Gerrit-Comment-Date: Tue, 15 Aug 2017 23:09:17 +0000 Gerrit-HasComments: Yes

1 0

Change in flashrom[staging]: Include <sys/types.h> wherever ssize_t is used
by build bot (Jenkins) (Code Review) 15 Aug '17

15 Aug '17

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/21015 ) Change subject: Include <sys/types.h> wherever ssize_t is used ...................................................................... Patch Set 1: Verified+1 Build Successful https://qa.coreboot.org/job/flashrom-customrules/508/ : SUCCESS https://qa.coreboot.org/job/flashrom_gerrit/468/ : SUCCESS -- To view, visit https://review.coreboot.org/21015 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Gerrit-Change-Number: 21015 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 15 Aug 2017 09:32:51 +0000 Gerrit-HasComments: No

1 0

Change in flashrom[staging]: Include <sys/types.h> wherever ssize_t is used
by Nico Huber (Code Review) 15 Aug '17

15 Aug '17

Nico Huber has uploaded this change for review. ( https://review.coreboot.org/21015 Change subject: Include <sys/types.h> wherever ssize_t is used ...................................................................... Include <sys/types.h> wherever ssize_t is used `ssize_t` is a POSIX type (cf. IEEE Std 1003.1). Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Signed-off-by: Nico Huber <nico.h(a)gmx.de> Reported-by: Urja Rannikko <urjaman(a)gmail.com> --- M dediprog.c M ich_descriptors.c M ich_descriptors.h 3 files changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/15/21015/1 diff --git a/dediprog.c b/dediprog.c index 6f82772..2f5b441 100644 --- a/dediprog.c +++ b/dediprog.c @@ -21,6 +21,7 @@ #include "platform.h" +#include <sys/types.h> #include <stdlib.h> #include <stdio.h> #include <string.h> diff --git a/ich_descriptors.c b/ich_descriptors.c index a2f8edf..7916152 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -34,6 +34,7 @@ #define UPPER_MAP_OFFSET (4096 - 256 - 4) #define getVTBA(flumap) (((flumap)->FLUMAP1 << 4) & 0x00000ff0) +#include <sys/types.h> #include <string.h> #include "flash.h" /* for msg_* */ #include "programmer.h" diff --git a/ich_descriptors.h b/ich_descriptors.h index 8c313d1..a3af8c2 100644 --- a/ich_descriptors.h +++ b/ich_descriptors.h @@ -22,6 +22,7 @@ #ifndef __ICH_DESCRIPTORS_H__ #define __ICH_DESCRIPTORS_H__ 1 +#include <sys/types.h> #include <stdint.h> #include "programmer.h" /* for enum ich_chipset */ -- To view, visit https://review.coreboot.org/21015 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: newchange Gerrit-Change-Id: I5f6f114523f541b3a8d845c6faee2c0b9f753bae Gerrit-Change-Number: 21015 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>

1 0

Change in flashrom[staging]: 4BA: Basic support for 4-bytes addressing mode extensions
by David Hendricks (Code Review) 14 Aug '17

14 Aug '17

David Hendricks has posted comments on this change. ( https://review.coreboot.org/20513 ) Change subject: 4BA: Basic support for 4-bytes addressing mode extensions ...................................................................... Patch Set 2: Code-Review+1 (1 comment) Either way is fine with me. If you have time and can edit this patch, I'll be happy to re-review and ack. There might be style issues in the follow-up patches, so perhaps doing a single !fixup would be the easiest/quickest thing to do here. https://review.coreboot.org/#/c/20513/1/spi4ba.c File spi4ba.c: https://review.coreboot.org/#/c/20513/1/spi4ba.c@127 PS1, Line 127: + 1) - 1 > > I'd eventually like to switch to using dynamically allocated memory That sounds good to me as well. -- To view, visit https://review.coreboot.org/20513 To unsubscribe, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: staging Gerrit-MessageType: comment Gerrit-Change-Id: Ie72e2a89cd75fb4d09f48e81c4c1d927c317b7a7 Gerrit-Change-Number: 20513 Gerrit-PatchSet: 2 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: Stefan Tauner <stefan.tauner(a)gmx.at> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Mon, 14 Aug 2017 06:50:13 +0000 Gerrit-HasComments: Yes

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit