| Message ID | 20240912013846.3058728-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Shuah Khan |
| Headers | show |
| Series | [1/2] pm: cpupower: bench: print path fopen failed | expand |
Hello Peng, These two seem like two separate patches and usually a series has a cover letter. Did you mean to send them separately or is something missing?
Hi John, > Subject: Re: [PATCH 1/2] pm: cpupower: bench: print path fopen failed > > Hello Peng, > > These two seem like two separate patches and usually a series has a > cover letter. Did you mean to send them separately or is something > missing? I think the two patches are just small patches, so send them together and not write cover-letter for them. I could write the cover-letter in v2 if there are any comments. Thanks, Peng. > > -- > Sincerely, > John Wyatt > Software Engineer, Core Kernel > Red Hat
On 9/11/24 19:38, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > Print out the config file path when fopen failed. It will be easy > for users to know where to create the file. Send these two patches as a series with a cover letter. Also what is changing - you can include what change: use the same subject line in here. The subject line can be improved to say more than fopen() failed. Which file open failed? The message can be informative about which file: about which file. e.g: pm: cpupower: bench: print config file path when open fails > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > tools/power/cpupower/bench/parse.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/power/cpupower/bench/parse.c b/tools/power/cpupower/bench/parse.c > index e63dc11fa3a5..366b20f9ddf1 100644 > --- a/tools/power/cpupower/bench/parse.c > +++ b/tools/power/cpupower/bench/parse.c > @@ -166,7 +166,7 @@ int prepare_config(const char *path, struct config *config) > configfile = fopen(path, "r"); > if (configfile == NULL) { > perror("fopen"); > - fprintf(stderr, "error: unable to read configfile\n"); > + fprintf(stderr, "error: unable to read configfile: %s\n", path); While you are at it, fix it to use strerror() instead of calling perror() followed by fprintf(). > free(config); > return 1; > } thanks, -- Shuah
> Subject: Re: [PATCH 1/2] pm: cpupower: bench: print path fopen failed > > On 9/11/24 19:38, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > Print out the config file path when fopen failed. It will be easy for > > users to know where to create the file. > > Send these two patches as a series with a cover letter. > > Also what is changing - you can include what change: use the same > subject line in here. > > The subject line can be improved to say more than fopen() failed. > Which file open failed? > > The message can be informative about which file: > about which file. > > e.g: pm: cpupower: bench: print config file path when open fails > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > tools/power/cpupower/bench/parse.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/power/cpupower/bench/parse.c > > b/tools/power/cpupower/bench/parse.c > > index e63dc11fa3a5..366b20f9ddf1 100644 > > --- a/tools/power/cpupower/bench/parse.c > > +++ b/tools/power/cpupower/bench/parse.c > > @@ -166,7 +166,7 @@ int prepare_config(const char *path, struct > config *config) > > configfile = fopen(path, "r"); > > if (configfile == NULL) { > > perror("fopen"); > > - fprintf(stderr, "error: unable to read configfile\n"); > > + fprintf(stderr, "error: unable to read configfile: %s\n", > path); > > While you are at it, fix it to use strerror() instead of calling perror() > followed by fprintf(). Seems the usage of perror is in the whole file. Could the conversion to sterror() be done in a separate patch? Thanks, Peng. > > > > free(config); > > return 1; > > } > > thanks, > -- Shuah
On 9/17/24 06:37, Peng Fan wrote: >> Subject: Re: [PATCH 1/2] pm: cpupower: bench: print path fopen failed >> >> On 9/11/24 19:38, Peng Fan (OSS) wrote: >>> From: Peng Fan <peng.fan@nxp.com> >>> >>> Print out the config file path when fopen failed. It will be easy for >>> users to know where to create the file. >> >> Send these two patches as a series with a cover letter. >> >> Also what is changing - you can include what change: use the same >> subject line in here. >> >> The subject line can be improved to say more than fopen() failed. >> Which file open failed? >> >> The message can be informative about which file: >> about which file. >> >> e.g: pm: cpupower: bench: print config file path when open fails >> >>> >>> Signed-off-by: Peng Fan <peng.fan@nxp.com> >>> --- >>> tools/power/cpupower/bench/parse.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/tools/power/cpupower/bench/parse.c >>> b/tools/power/cpupower/bench/parse.c >>> index e63dc11fa3a5..366b20f9ddf1 100644 >>> --- a/tools/power/cpupower/bench/parse.c >>> +++ b/tools/power/cpupower/bench/parse.c >>> @@ -166,7 +166,7 @@ int prepare_config(const char *path, struct >> config *config) >>> configfile = fopen(path, "r"); >>> if (configfile == NULL) { >>> perror("fopen"); >>> - fprintf(stderr, "error: unable to read configfile\n"); >>> + fprintf(stderr, "error: unable to read configfile: %s\n", >> path); >> >> While you are at it, fix it to use strerror() instead of calling perror() >> followed by fprintf(). > > Seems the usage of perror is in the whole file. Could the conversion > to sterror() be done in a separate patch? > Yes. That can be a separate patch. Please send them together in a patch series. I will pull those in after the merge window closes. thanks, -- Shuah
diff --git a/tools/power/cpupower/bench/parse.c b/tools/power/cpupower/bench/parse.c index e63dc11fa3a5..366b20f9ddf1 100644 --- a/tools/power/cpupower/bench/parse.c +++ b/tools/power/cpupower/bench/parse.c @@ -166,7 +166,7 @@ int prepare_config(const char *path, struct config *config) configfile = fopen(path, "r"); if (configfile == NULL) { perror("fopen"); - fprintf(stderr, "error: unable to read configfile\n"); + fprintf(stderr, "error: unable to read configfile: %s\n", path); free(config); return 1; }