Re: Fix multiple compiler warnings when building with gcc-12
On 1/4/23 08:52, Nathan Lynch wrote: > John Paul Adrian Glaubitz writes: > >> Ping. Any chance we can get these small fixes merged for the next >> branch? > > Tyrel, I think these all look good. Thanks Adrian. > > We should really remove the -Werror from the default build flags, and > instead use it judiciously in the CI. Agreed. Thanks Nathan for the immediate turn around on patches to do just that. -Tyrel
Re: Fix multiple compiler warnings when building with gcc-12
On 12/26/22 01:54, John Paul Adrian Glaubitz wrote: > This patch series fixes a number of warnings when building powerpc-utils > with gcc-12 or newer. Since the project builds with "-Werror" by default, > these warnings will cause the build to fail. > > With these patches applied, all warnings are gone when building on ppc64el, > there are two additional warnings left regarding possibly truncated strings > when building on big-endian PowerPC targets for which I will send a separate > patch set. > > Cheers, > Adrian > > -- Patchset applied to powerpc-utils/next branch: [1/3] https://github.com/ibm-power-utilities/powerpc-utils/commit/aef8f14ed8b241ab66d88fb9a5aeefe47a847267 [2/3] https://github.com/ibm-power-utilities/powerpc-utils/commit/5de0a4a070981b5ee005f2242b31db5422be297a [3/3] https://github.com/ibm-power-utilities/powerpc-utils/commit/3607e6dabdef641c363233eddd3a1cf8c2e5c6d8 Thanks, -Tyrel
Re: Fix multiple compiler warnings when building with gcc-12
On 12/26/22 01:54, John Paul Adrian Glaubitz wrote: > This patch series fixes a number of warnings when building powerpc-utils > with gcc-12 or newer. Since the project builds with "-Werror" by default, > these warnings will cause the build to fail. > > With these patches applied, all warnings are gone when building on ppc64el, > there are two additional warnings left regarding possibly truncated strings > when building on big-endian PowerPC targets for which I will send a separate > patch set. I ran our CI with the latest Ubuntu LTS runner which has the gcc-11 toolchain and I see the two truncated string warnings with that toolchain. -Tyrel > > Cheers, > Adrian > > -- > .''`. John Paul Adrian Glaubitz > : :' : Debian Developer > `. `' Physicist > `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913 > >
Re: Fix multiple compiler warnings when building with gcc-12
On 1/5/23 13:07, John Paul Adrian Glaubitz wrote:
> Hi Tyrel!
>
> On 1/5/23 21:58, Tyrel Datwyler wrote:
>> On 12/26/22 01:54, John Paul Adrian Glaubitz wrote:
>>> This patch series fixes a number of warnings when building powerpc-utils
>>> with gcc-12 or newer. Since the project builds with "-Werror" by default,
>>> these warnings will cause the build to fail.
>>>
>>> With these patches applied, all warnings are gone when building on ppc64el,
>>> there are two additional warnings left regarding possibly truncated strings
>>> when building on big-endian PowerPC targets for which I will send a separate
>>> patch set.
>>
>> I ran our CI with the latest Ubuntu LTS runner which has the gcc-11 toolchain
>> and I see the two truncated string warnings with that toolchain.
>
> So, these two warnings actually go away when I replace strncpy() with memcpy()
> but I have to admit, I don't fully understand why that's the case.
In cases where the compiler can extrapolate the size of the source buffer and
the value or value range of `n` we get a possible truncation warning when `n` or
the range of `n` is less than the length of the source buffer.
By using memcpy the compiler no longer assumes the source is a string and we
are claiming we know what we are doing and that the exact range of `n` should be
copied. This should probably raise some eyebrows with regards to string
handling. To use memcpy we need be explicitly terminating or ensuring by some
means that a string range copied with memcpy is guaranteed to be NULL
terminated.
A lot of this code base I think has implicit assumptions about what a reasonably
big buffer size should be to prevent overrun-truncation.
So, I'm a little hesitant to switch to memcpy to silence these warnings as
digging deeper into them has revealed some unexpectedly subtle bugs.
>
> diff --git a/src/errinjct/ioa_bus_error.c b/src/errinjct/ioa_bus_error.c
> index 9d85cfa..5ee1401 100644
> --- a/src/errinjct/ioa_bus_error.c
> +++ b/src/errinjct/ioa_bus_error.c
> @@ -204,7 +204,7 @@ static uint32_t get_config_addr_from_reg(char *devpath)
> uint32_t *be_caddr;
> uint32_t caddr = 0;
>
> - strncpy(path, devpath, BUFSZ-5);
> + memcpy(path, devpath, BUFSZ-5);
> strcat(path, "/reg");
The caller passes the devpath buffer which is allocated statically as BUFSZ.
Since, we try to copy only BUFSZ-5 to ensure we have space to strcat the string
"/reg\0" we get the truncation warning. Using memcpy here or changing the static
allocation size of devpath in the caller to BUFSZ-5 both make the warning go
away.
Second to this however is that the errinjct code defines BUFSZ as 4000. This is
undersized if we somehow ever hit a maximum filepath on Linux where PATH_MAX is
4096.
>
> buf = read_file(path, NULL);
> diff --git a/src/serv_config.c b/src/serv_config.c
> index 00ab672..2565533 100644
> --- a/src/serv_config.c
> +++ b/src/serv_config.c
> @@ -707,7 +707,7 @@ retrieve_value(struct service_var *var, char *buf, size_t
> size) {
> byte_to_string(param[2], buf, size);
> }
> else {
> - strncpy(buf, param+2, ((size>ret_size)?
> + memcpy(buf, param+2, ((size>ret_size)?
> ret_size:size));
> buf[ret_size] = '\0';
> }
This one is interesting and is possibly a good candidate for being switched over
to memcpy. The compiler knows `param` is statically allocated as BUF_SIZE, and
that the value of `size` passed in by the caller is BUF_SIZE. The first two
bytes of param are the actual string length of the string the makes up the rest
of the param buffer and is copied into `ret_size` variable. So, `param+2` is a
buffer of size BUF_SIZE-2 which is less than BUF_SIZE and when we use `size`
with strncpy it actually copies 2 random bytes passed the end of `param` into
`buf` if no NULL terminator is found. I haven't dug deep enough to determine if
the string returned in `param` is guaranteed to be NULL terminated, but
considering there is an explicit NULL termination in the code I don't think it
is safe to assume.
The most interesting part is why the compiler thinks `ret_size` is a range of
0-255 when it is a uint16, and creates the truncation warning since the range is
smaller than BUF_SIZE-2.
690 ret_size = be16toh(*param);
This byte swap code found a little before is wrong and will only store the first
byte of `param` since param is defined as a char array. So, this code is
actually broken if a parameter string longer than 255 characters is ever
returned.
Re: Ping: Re: [PATCH]: Add support for Apple Power Mac
On 6/5/20 7:15 AM, Nathan Lynch wrote: > John Paul Adrian Glaubitz writes: >> Hi! >> >> On 5/17/20 12:33 PM, John Paul Adrian Glaubitz wrote: >>> On 4/19/20 11:35 AM, John Paul Adrian Glaubitz wrote: The following three patches are required to get ofpathname to return proper OF paths on Apple Power Macs. >>> >>> Any chance someone could have a look at this? >> >> Gentle ping. > > Thanks for your persistence. Tyrel, I've looked these over and didn't > see anything wrong. > Thanks, for getting an extra set of eyes on these Nathan. I'll add your reviewed-by when I merge. -Tyrel
Re: [PATCH 3/3] ofpathname: Add support for Mac-compatible OF pathnames
On 4/19/20 2:35 AM, John Paul Adrian Glaubitz wrote: > On Macintosh systems, OpenFirmware path follow a slightly > different syntax as compared to other OF systems. In particular, > the disk ID is not preceeded by the string "disk" and - on > SATA systems - the path does not contain the BUS ID as > "scsi@ID" but rather the plug ID in the form "@ID". > > Signed-off-by: John Paul Adrian Glaubitz > --- Series applied to powerpc-utils next. https://github.com/ibm-power-utilities/powerpc-utils/commit/f8f2f09c413838fe9f0b30478de5d08bdb3e95aa Thanks, Tyrel
Re: [PATCH 1/3] ofpathname: Add partition support to l2of_ide() and l2of_scsi()
On 4/19/20 2:35 AM, John Paul Adrian Glaubitz wrote:
> Currently, only l2of_nvme() supports handling of partitions which
> will print the partition number at the end of an OF path. However,
> this feature is also useful for IDE and SCSI targets as well and
> mandatory for Macintosh systems.
>
> Signed-off-by: John Paul Adrian Glaubitz
> ---
> scripts/ofpathname | 27 +++
> 1 file changed, 27 insertions(+)
>
> diff --git a/scripts/ofpathname b/scripts/ofpathname
> index c37c6bd..0e58005 100755
> --- a/scripts/ofpathname
> +++ b/scripts/ofpathname
> @@ -519,6 +519,13 @@ l2of_ide()
> if [[ -z $link ]]; then
> err $ERR_NO_SYSFS_DEVINFO
> fi
> +
> +# partition number: N in sd*N
> +local devpart="${DEVICE##*[a-z]}"
> +if [[ $devpart = $DEVICE ]]; then
> +devpart='' # no partition number
> +fi
> +
> cd $link
>
> # get the device number
> @@ -584,6 +591,13 @@ l2of_vd()
> if [[ -z $OF_PATH ]]; then
> err $ERR_NO_OFPATH
> fi
> +
> +# No partition specified.
> +if [[ -z $devpart ]]; then
> +return
> +fi
> +
> +OF_PATH="${OF_PATH}:${devpart}"
> }
>
> #
> @@ -786,6 +800,12 @@ l2of_scsi()
> err $ERR_NOT_CONFIG
> fi
>
> +# partition number: N in sd*N
> +local devpart="${DEVICE##*[a-z]}"
> +if [[ $devpart = $DEVICE ]]; then
> +devpart='' # no partition number
> +fi
> +
> # follow the 'device' link
> local link=`get_link "device"`
> if [[ -z $link ]]; then
> @@ -944,6 +964,13 @@ l2of_scsi()
> OF_PATH=$OF_PATH/sd@$ID,$LUN
> fi
> fi
> +
> +# No partition specified.
> +if [[ -z $devpart ]]; then
> +return
> +fi
> +
> +OF_PATH="${OF_PATH}:${devpart}"
Heads up this may be problematic for pseries lpars. The current behavior is that
regardless of the scsi partition the logical device always translates to the
full underlying disk. I will need to confirm whether OF is going to complain
when the partition number is appended.
-Tyrel
> }
>
> #
>
Re: [PATCH 1/3] ofpathname: Add partition support to l2of_ide() and l2of_scsi()
On 6/22/20 10:43 PM, John Paul Adrian Glaubitz wrote: > Hi! > > On 6/23/20 3:32 AM, Tyrel Datwyler wrote: >> Heads up this may be problematic for pseries lpars. The current behavior is >> that >> regardless of the scsi partition the logical device always translates to the >> full underlying disk. I will need to confirm whether OF is going to complain >> when the partition number is appended. > > If that should be a problem, we can enable this on the Mac platform only. > Although > I'm surprised this should be a problem since the previously used tools like > ofpath > from Yaboot behaved exactly like that and I'm not aware of any problems with > Yaboot > on these machines. Ok, so on the bright side I've confirmed that IBM open firmware will boot a pseries machine when the partition ID is provided and points at the actual PReP bootstrap partition. I suspect it was just generally omitted because it was assumed that PReP would always be the first partition historically. I believe these days that if it isn't the first partition OF is smart enough to locate it on its own which seems to further the lack of a need to explicitly provide the partition ID on this platforms. Now the downside is that ofpathname is not able to translate a OF vscsi device path to a logical device if that OF path includes the partition ID. So, I'm still inclined to say that the changes in this patch should be limited to the Mac platform for the time being. -Tyrel > > Adrian >
Re: [PATCH, v2] ofpathname: Move definition of SYS_PATH from l2of_vs() to l2of_scsi()
On 2/8/21 4:10 PM, John Paul Adrian Glaubitz wrote:
> SYS_PATH, which is required to calculate the plug ID of a SCSI/SATA
> host in l2of_scsi(), is actually never set in this function but in
> l2of_vd() where it is not used at all. Thus, move the definition of
> SYS_PATH from l2of_vd() to l2of_scsi() to fix the calculation of the
> plug ID in l2of_scsi().
>
> Fixes: 3fb2c44e22 ("ofpathname: Add support for the plug ID of a SCSI/SATA
> host")
> Signed-off-by: John Paul Adrian Glaubitz
> ---
> v2:
> - improve phrasing in commit message
Applied to powerpc-utils/next branch.
https://github.com/ibm-power-utilities/powerpc-utils/commit/67e974dbb8e4462c69c9c0e3a3b0c3a7219a2d45
Thanks,
Tyrel
Re: [PATCH] ofpathname: Add missing substring extraction of devpart in l2of_vd()
On 4/18/21 1:05 AM, John Paul Adrian Glaubitz wrote: > l2of_vd() contains the necessary bits to append the partition number > to the resulting OFPATH, but it does not actually extract the partition > number from the logical device path in the first place. This adds the > missing substring extraction of the partition number from the logical > device path so that the partition number is appended to OFPATH when > the logical device is a partition. > > Signed-off-by: John Paul Adrian Glaubitz > --- Patch applied to powerpc-utils/next branch. https://github.com/ibm-power-utilities/powerpc-utils/commit/9104167a0ee93989757326136b94daad585cd87c Thanks, -Tyrel
