* [pve-devel] [PATCH storage] fix #3203: report smart status correctly
@ 2021-03-01 13:56 Oguz Bektas
2021-03-01 16:40 ` Dietmar Maurer
2021-03-02 8:34 ` Dominik Csapak
0 siblings, 2 replies; 4+ messages in thread
From: Oguz Bektas @ 2021-03-01 13:56 UTC (permalink / raw)
To: pve-devel
if the -a option isn't passed, -H might report a failing disk as
'PASSED' even when the disk might be in a corrupted state.
Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
---
PVE/Diskmanage.pm | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
index 64bb813..d3c3365 100644
--- a/PVE/Diskmanage.pm
+++ b/PVE/Diskmanage.pm
@@ -95,8 +95,9 @@ sub get_smart_data {
or die "failed to get nvme controller device for $disk\n");
}
- my $cmd = [$SMARTCTL, '-H'];
- push @$cmd, '-A', '-f', 'brief' if !$healthonly;
+ my $cmd = [$SMARTCTL];
+ push @$cmd, '-H' if $healthonly;
+ push @$cmd, '-a', '-A', '-f', 'brief' if !$healthonly;
push @$cmd, $disk;
eval {
--
2.20.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH storage] fix #3203: report smart status correctly
2021-03-01 13:56 [pve-devel] [PATCH storage] fix #3203: report smart status correctly Oguz Bektas
@ 2021-03-01 16:40 ` Dietmar Maurer
2021-03-02 8:34 ` Dominik Csapak
1 sibling, 0 replies; 4+ messages in thread
From: Dietmar Maurer @ 2021-03-01 16:40 UTC (permalink / raw)
To: Proxmox VE development discussion, Oguz Bektas
This code is quite strange. Please can you use a
normal: if .. then .. else ..
> + push @$cmd, '-H' if $healthonly;
> + push @$cmd, '-a', '-A', '-f', 'brief' if !$healthonly;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH storage] fix #3203: report smart status correctly
2021-03-01 13:56 [pve-devel] [PATCH storage] fix #3203: report smart status correctly Oguz Bektas
2021-03-01 16:40 ` Dietmar Maurer
@ 2021-03-02 8:34 ` Dominik Csapak
2021-03-02 12:33 ` Oguz Bektas
1 sibling, 1 reply; 4+ messages in thread
From: Dominik Csapak @ 2021-03-02 8:34 UTC (permalink / raw)
To: pve-devel
On 3/1/21 14:56, Oguz Bektas wrote:
> if the -a option isn't passed, -H might report a failing disk as
> 'PASSED' even when the disk might be in a corrupted state.
>
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
> PVE/Diskmanage.pm | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
> index 64bb813..d3c3365 100644
> --- a/PVE/Diskmanage.pm
> +++ b/PVE/Diskmanage.pm
> @@ -95,8 +95,9 @@ sub get_smart_data {
> or die "failed to get nvme controller device for $disk\n");
> }
>
> - my $cmd = [$SMARTCTL, '-H'];
> - push @$cmd, '-A', '-f', 'brief' if !$healthonly;
> + my $cmd = [$SMARTCTL];
> + push @$cmd, '-H' if $healthonly;
> + push @$cmd, '-a', '-A', '-f', 'brief' if !$healthonly;
> push @$cmd, $disk;
>
> eval {
>
after looking at our code, this patch does not solve the issue the
user in #3203 has
we purposely ignore the return value of smartctl except
for sever smartctl errors (such as device open failed or faulty command
line) and parse the status from the text itself
but the text (PASSED) does not change regardless if -H is given or not
the real fix for the issue is to start parsing the smart errors
separately and show them to the user somehow
while doing this, we should change our parser to use the '--json' flag
of smartcl
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH storage] fix #3203: report smart status correctly
2021-03-02 8:34 ` Dominik Csapak
@ 2021-03-02 12:33 ` Oguz Bektas
0 siblings, 0 replies; 4+ messages in thread
From: Oguz Bektas @ 2021-03-02 12:33 UTC (permalink / raw)
To: Proxmox VE development discussion
hi,
On Tue, Mar 02, 2021 at 09:34:41AM +0100, Dominik Csapak wrote:
> On 3/1/21 14:56, Oguz Bektas wrote:
> > if the -a option isn't passed, -H might report a failing disk as
> > 'PASSED' even when the disk might be in a corrupted state.
> >
> > Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> > ---
> > PVE/Diskmanage.pm | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/PVE/Diskmanage.pm b/PVE/Diskmanage.pm
> > index 64bb813..d3c3365 100644
> > --- a/PVE/Diskmanage.pm
> > +++ b/PVE/Diskmanage.pm
> > @@ -95,8 +95,9 @@ sub get_smart_data {
> > or die "failed to get nvme controller device for $disk\n");
> > }
> > - my $cmd = [$SMARTCTL, '-H'];
> > - push @$cmd, '-A', '-f', 'brief' if !$healthonly;
> > + my $cmd = [$SMARTCTL];
> > + push @$cmd, '-H' if $healthonly;
> > + push @$cmd, '-a', '-A', '-f', 'brief' if !$healthonly;
> > push @$cmd, $disk;
> > eval {
> >
>
> after looking at our code, this patch does not solve the issue the
> user in #3203 has
>
> we purposely ignore the return value of smartctl except
> for sever smartctl errors (such as device open failed or faulty command
> line) and parse the status from the text itself
>
> but the text (PASSED) does not change regardless if -H is given or not
>
> the real fix for the issue is to start parsing the smart errors separately
> and show them to the user somehow
hmm yeah you're right, thanks!
>
> while doing this, we should change our parser to use the '--json' flag
> of smartcl
i will take a look. like you also mentioned off-list we do this in
backup server code with the json flag, so it makes sense to do it here
as well
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-02 12:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-01 13:56 [pve-devel] [PATCH storage] fix #3203: report smart status correctly Oguz Bektas
2021-03-01 16:40 ` Dietmar Maurer
2021-03-02 8:34 ` Dominik Csapak
2021-03-02 12:33 ` Oguz Bektas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox