* Re: [pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type
[not found] <20240607094734.3883693-1-jing@jing.rocks>
@ 2024-06-07 11:49 ` Fiona Ebner
2024-06-10 1:48 ` Jing Luo via pve-devel
[not found] ` <57a95560b7c8bce55d2e8270d18d9778@jing.rocks>
2024-06-10 8:53 ` [pve-devel] [PATCH pve-qemu-kvm] patch: " Shannon Sterz
1 sibling, 2 replies; 7+ messages in thread
From: Fiona Ebner @ 2024-06-07 11:49 UTC (permalink / raw)
To: Jing Luo, pve-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type
2024-06-07 11:49 ` [pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type Fiona Ebner
@ 2024-06-10 1:48 ` Jing Luo via pve-devel
[not found] ` <57a95560b7c8bce55d2e8270d18d9778@jing.rocks>
1 sibling, 0 replies; 7+ messages in thread
From: Jing Luo via pve-devel @ 2024-06-10 1:48 UTC (permalink / raw)
To: Fiona Ebner; +Cc: Jing Luo, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type
[not found] <20240607094734.3883693-1-jing@jing.rocks>
2024-06-07 11:49 ` [pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type Fiona Ebner
@ 2024-06-10 8:53 ` Shannon Sterz
1 sibling, 0 replies; 7+ messages in thread
From: Shannon Sterz @ 2024-06-10 8:53 UTC (permalink / raw)
To: Jing Luo, pve-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type
[not found] ` <57a95560b7c8bce55d2e8270d18d9778@jing.rocks>
@ 2024-06-10 9:27 ` Fiona Ebner
2024-06-10 12:05 ` [pve-devel] [PATCH v2 pve-qemu-kvm] patches: " Jing Luo via pve-devel
[not found] ` <20240610122909.1216178-2-jing@jing.rocks>
0 siblings, 2 replies; 7+ messages in thread
From: Fiona Ebner @ 2024-06-10 9:27 UTC (permalink / raw)
To: Jing Luo; +Cc: pve-devel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] [PATCH v2 pve-qemu-kvm] patches: pbs block driver: correct a data type
2024-06-10 9:27 ` Fiona Ebner
@ 2024-06-10 12:05 ` Jing Luo via pve-devel
[not found] ` <20240610122909.1216178-2-jing@jing.rocks>
1 sibling, 0 replies; 7+ messages in thread
From: Jing Luo via pve-devel @ 2024-06-10 12:05 UTC (permalink / raw)
To: pve-devel; +Cc: Jing Luo
[-- Attachment #1: Type: message/rfc822, Size: 7644 bytes --]
From: Jing Luo <jing@jing.rocks>
To: pve-devel@lists.proxmox.com
Cc: f.ebner@proxmox.com, Jing Luo <jing@jing.rocks>
Subject: [PATCH v2 pve-qemu-kvm] patches: pbs block driver: correct a data type
Date: Mon, 10 Jun 2024 21:05:26 +0900
Message-ID: <20240610122909.1216178-2-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.
This is probably because chars are treated as unsigned on arm arch but
signed on x86 arch:
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>
--
[changes since v1]
Use uint8_t for s->aid. Check the returned value from
proxmox_restore_open_image(), then assign it to s->aid.
Fiona: the doctor found no fish bone in my throat (probably swallowed),
so here's my best try :) I did minimal testing (it built).
I also found that chars are unsigned on riscv64, powerpc64le...not that
proxomx needs to worry about those arch though.
---
...ock-driver-to-map-backup-archives-into.patch | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
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..4c155c8 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
@@ -15,11 +15,11 @@ Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
block/meson.build | 2 +
- block/pbs.c | 307 +++++++++++++++++++++++++++++++++++++++++++
+ block/pbs.c | 310 +++++++++++++++++++++++++++++++++++++++++++
meson.build | 2 +-
qapi/block-core.json | 29 ++++
qapi/pragma.json | 1 +
- 5 files changed, 340 insertions(+), 1 deletion(-)
+ 5 files changed, 343 insertions(+), 1 deletion(-)
create mode 100644 block/pbs.c
diff --git a/block/meson.build b/block/meson.build
@@ -37,10 +37,10 @@ index 6bba803f94..1945e04eeb 100644
system_ss.add(files('block-ram-registrar.c'))
diff --git a/block/pbs.c b/block/pbs.c
new file mode 100644
-index 0000000000..dd72356bd3
+index 0000000000..9112d4dfe6
--- /dev/null
+++ b/block/pbs.c
-@@ -0,0 +1,307 @@
+@@ -0,0 +1,310 @@
+/*
+ * Proxmox Backup Server read-only block driver
+ */
@@ -68,7 +68,7 @@ index 0000000000..dd72356bd3
+
+typedef struct {
+ ProxmoxRestoreHandle *conn;
-+ char aid;
++ uint8_t aid;
+ int64_t length;
+
+ char *repository;
@@ -201,12 +201,15 @@ index 0000000000..dd72356bd3
+ }
+
+ /* acquire handle and length */
-+ s->aid = proxmox_restore_open_image(s->conn, s->archive, &pbs_error);
-+ if (s->aid < 0) {
++ ret = proxmox_restore_open_image(s->conn, s->archive, &pbs_error);
++ if (ret < 0 || ret > UINT8_MAX) {
+ if (pbs_error && errp) error_setg(errp, "PBS open_image failed: %s", pbs_error);
+ if (pbs_error) proxmox_backup_free_error(pbs_error);
+ return -ENODEV;
++ } else {
++ s->aid = ret;
+ }
++
+ s->length = proxmox_restore_get_image_length(s->conn, s->aid, &pbs_error);
+ if (s->length < 0) {
+ if (pbs_error && errp) error_setg(errp, "PBS get_image_length failed: %s", pbs_error);
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] applied: [PATCH v2 pve-qemu-kvm] patches: pbs block driver: correct a data type
[not found] ` <20240610122909.1216178-2-jing@jing.rocks>
@ 2024-06-10 14:12 ` Fiona Ebner
0 siblings, 0 replies; 7+ messages in thread
From: Fiona Ebner @ 2024-06-10 14:12 UTC (permalink / raw)
To: Jing Luo, pve-devel
Am 10.06.24 um 14:05 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.
>
> This is probably because chars are treated as unsigned on arm arch but
> signed on x86 arch:
>
> 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>
> --
applied, thanks!
Here, a '-' was missing, so the below stuff would still be interpreted
as part of the commit message. I fixed that when applying, improved the
message slightly and made a follow-up to have a dedicated error message
for when the returned value is too large.
Best Regards,
Fiona
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type
@ 2024-06-07 9:43 Jing Luo via pve-devel
0 siblings, 0 replies; 7+ messages in thread
From: Jing Luo via pve-devel @ 2024-06-07 9:43 UTC (permalink / raw)
To: pve-devel; +Cc: Jing Luo
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-06-11 10:56 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20240607094734.3883693-1-jing@jing.rocks>
2024-06-07 11:49 ` [pve-devel] [PATCH pve-qemu-kvm] patch: pbs block driver: correct a data type Fiona Ebner
2024-06-10 1:48 ` Jing Luo via pve-devel
[not found] ` <57a95560b7c8bce55d2e8270d18d9778@jing.rocks>
2024-06-10 9:27 ` Fiona Ebner
2024-06-10 12:05 ` [pve-devel] [PATCH v2 pve-qemu-kvm] patches: " Jing Luo via pve-devel
[not found] ` <20240610122909.1216178-2-jing@jing.rocks>
2024-06-10 14:12 ` [pve-devel] applied: " Fiona Ebner
2024-06-10 8:53 ` [pve-devel] [PATCH pve-qemu-kvm] patch: " Shannon Sterz
2024-06-07 9:43 Jing Luo via pve-devel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox