[pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type

[pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type

[Jing Luo via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 5444 bytes --]

From: Jing Luo <jing@jing.rocks>
To: pve-devel@lists.proxmox.com
Cc: Jing Luo <jing@jing.rocks>
Subject: [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type
Date: Fri,  7 Jun 2024 18:43:23 +0900
Message-ID: <20240607094734.3883693-1-jing@jing.rocks>

gcc warns (-Werror=type-limits) that it will always be false for the
if statement. This is because here s->aid is defined as char, while
proxmox_restore_open_image() returns an int. Change the type to int.
Strangely gcc warns it on arm64 build but not amd64 build...

Signed-off-by: Jing Luo <jing@jing.rocks>
---
 ...2-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch b/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
index b9578ba..9e68167 100644
--- a/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
+++ b/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
@@ -68,7 +68,7 @@ index 0000000000..dd72356bd3
 +
 +typedef struct {
 +    ProxmoxRestoreHandle *conn;
-+    char aid;
++    int aid;
 +    int64_t length;
 +
 +    char *repository;
-- 
2.45.2




[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type

[Fiona Ebner]

Am 10.06.24 um 03:48 schrieb Jing Luo:
> On 2024-06-07 20:49, Fiona Ebner wrote:
>> Hi,
>>
>> if you haven't already done so, please send a signed copy of the Harmony
>> CLA to office@proxmox.com, see [0].
> 
> Hi, thanks for letting me know, this was done some time last week.
> 

Okay :)

>> Am 07.06.24 um 11:43 schrieb Jing Luo:
>>> gcc warns (-Werror=type-limits) that it will always be false for the
>>> if statement. This is because here s->aid is defined as char, while
>>> proxmox_restore_open_image() returns an int. Change the type to int.
>>> Strangely gcc warns it on arm64 build but not amd64 build...
>>>
>>
>> Thank you for the report!
>>
>>> Signed-off-by: Jing Luo <jing@jing.rocks>
>>> ---
>>>  ...2-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git
>>> a/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch b/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
>>> index b9578ba..9e68167 100644
>>> ---
>>> a/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
>>> +++
>>> b/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
>>> @@ -68,7 +68,7 @@ index 0000000000..dd72356bd3
>>>  +
>>>  +typedef struct {
>>>  +    ProxmoxRestoreHandle *conn;
>>> -+    char aid;
>>> ++    int aid;
>>>  +    int64_t length;
>>>  +
>>>  +    char *repository;
>>
>> I'd rather make it an explicit uint8_t here (because that is the type
>> for other functions taking the aid as a parameter, e.g.
>> proxmox_restore_get_image_length()). And to fix the original issue, I'd
>> use the ret variable to store the result from
>> proxmox_restore_open_image() and only assign to s->aid after checking
>> that the returned value is not an error and that it is small enough to
>> fit into uint8_t.
>>
>> [0]:
>> https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright
> 
> Thanks. I'm more of a sysadmin than a programmer but I'll try my best. I
> might send a v2 patch in a few days. (Unless I need an urgent surgery to
> take out the fish bone deeply stuck in my throat since last
> Friday...I'll find out if I need the surgery after meeting with my
> doctor today:)

No need to hurry, and if you want to, I can also send a patch for this
with a Reported-by tag for you. Hope all goes well regarding the medical
issue!

Best Wishes,
Fiona


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type

[Shannon Sterz]

On Fri Jun 7, 2024 at 11:43 AM CEST, Jing Luo wrote:
> gcc warns (-Werror=type-limits) that it will always be false for the
> if statement. This is because here s->aid is defined as char, while
> proxmox_restore_open_image() returns an int. Change the type to int.
> Strangely gcc warns it on arm64 build but not amd64 build...

probably because char is unsigned on arm architectures (usually), but
signed on x86 [1]:

> Although the ARM architecture has the LDRSB instruction, that loads a
> signed byte into a 32-bit register with sign extension, the earliest
> versions of the architecture did not. It made sense at the time for
> the compiler to treat simple chars as unsigned, whereas on the x86
> simple chars are, by default, treated as signed.

[1]: https://developer.arm.com/documentation/den0013/d/Porting/Miscellaneous-C-porting-issues/unsigned-char-and-signed-char

>
> Signed-off-by: Jing Luo <jing@jing.rocks>
> ---
>  ...2-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch b/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
> index b9578ba..9e68167 100644
> --- a/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
> +++ b/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
> @@ -68,7 +68,7 @@ index 0000000000..dd72356bd3
>  +
>  +typedef struct {
>  +    ProxmoxRestoreHandle *conn;
> -+    char aid;
> ++    int aid;
>  +    int64_t length;
>  +
>  +    char *repository;



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type

[Jing Luo via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 7515 bytes --]

[-- Attachment #1.1.1: Type: text/plain, Size: 2331 bytes --]

On 2024-06-07 20:49, Fiona Ebner wrote:
> Hi,
> 
> if you haven't already done so, please send a signed copy of the 
> Harmony
> CLA to office@proxmox.com, see [0].

Hi, thanks for letting me know, this was done some time last week.

> Am 07.06.24 um 11:43 schrieb Jing Luo:
>> gcc warns (-Werror=type-limits) that it will always be false for the
>> if statement. This is because here s->aid is defined as char, while
>> proxmox_restore_open_image() returns an int. Change the type to int.
>> Strangely gcc warns it on arm64 build but not amd64 build...
>> 
> 
> Thank you for the report!
> 
>> Signed-off-by: Jing Luo <jing@jing.rocks>
>> ---
>>  ...2-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch | 2 
>> +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git 
>> a/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch 
>> b/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
>> index b9578ba..9e68167 100644
>> --- 
>> a/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
>> +++ 
>> b/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
>> @@ -68,7 +68,7 @@ index 0000000000..dd72356bd3
>>  +
>>  +typedef struct {
>>  +    ProxmoxRestoreHandle *conn;
>> -+    char aid;
>> ++    int aid;
>>  +    int64_t length;
>>  +
>>  +    char *repository;
> 
> I'd rather make it an explicit uint8_t here (because that is the type
> for other functions taking the aid as a parameter, e.g.
> proxmox_restore_get_image_length()). And to fix the original issue, I'd
> use the ret variable to store the result from
> proxmox_restore_open_image() and only assign to s->aid after checking
> that the returned value is not an error and that it is small enough to
> fit into uint8_t.
> 
> [0]:
> https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright

Thanks. I'm more of a sysadmin than a programmer but I'll try my best. I 
might send a v2 patch in a few days. (Unless I need an urgent surgery to 
take out the fish bone deeply stuck in my throat since last 
Friday...I'll find out if I need the surgery after meeting with my 
doctor today:)


-- 
Jing Luo
About me: https://jing.rocks/about/
PGP Fingerprint: 4E09 8D19 00AA 3F72 1899 2614 09B3 316E 13A1 1EFC

[-- Attachment #1.1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type

[Fiona Ebner]

Hi,

if you haven't already done so, please send a signed copy of the Harmony
CLA to office@proxmox.com, see [0].

Am 07.06.24 um 11:43 schrieb Jing Luo:
> gcc warns (-Werror=type-limits) that it will always be false for the
> if statement. This is because here s->aid is defined as char, while
> proxmox_restore_open_image() returns an int. Change the type to int.
> Strangely gcc warns it on arm64 build but not amd64 build...
> 

Thank you for the report!

> Signed-off-by: Jing Luo <jing@jing.rocks>
> ---
>  ...2-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch b/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
> index b9578ba..9e68167 100644
> --- a/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
> +++ b/debian/patches/pve/0032-PVE-Add-PBS-block-driver-to-map-backup-archives-into.patch
> @@ -68,7 +68,7 @@ index 0000000000..dd72356bd3
>  +
>  +typedef struct {
>  +    ProxmoxRestoreHandle *conn;
> -+    char aid;
> ++    int aid;
>  +    int64_t length;
>  +
>  +    char *repository;

I'd rather make it an explicit uint8_t here (because that is the type
for other functions taking the aid as a parameter, e.g.
proxmox_restore_get_image_length()). And to fix the original issue, I'd
use the ret variable to store the result from
proxmox_restore_open_image() and only assign to s->aid after checking
that the returned value is not an error and that it is small enough to
fit into uint8_t.

[0]:
https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright

Best Regards,
Fiona


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel