all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH storage] style: remove goto statements
@ 2024-06-26  8:56 Fabian Grünbichler
  2024-06-27 13:28 ` Fiona Ebner
  0 siblings, 1 reply; 3+ messages in thread
From: Fabian Grünbichler @ 2024-06-26  8:56 UTC (permalink / raw)
  To: pve-devel

these can just as well be `die` statements right there, there is no complicated
cleanup that would warrant a goto statement..

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/Storage/Plugin.pm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index b5a54c1..f8dc9a2 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1586,13 +1586,14 @@ sub read_common_header($) {
 # Export a volume into a file handle as a stream of desired format.
 sub volume_export {
     my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots) = @_;
+    my $unsupported = "volume export format $format not available for $class\n";
     if ($scfg->{path} && !defined($snapshot) && !defined($base_snapshot)) {
 	my $file = $class->path($scfg, $volname, $storeid)
-	    or goto unsupported;
+	    or die $unsupported;
 	my ($size, $file_format) = file_size_info($file);
 
 	if ($format eq 'raw+size') {
-	    goto unsupported if $with_snapshots || $file_format eq 'subvol';
+	    die $unsupported if $with_snapshots || $file_format eq 'subvol';
 	    write_common_header($fh, $size);
 	    if ($file_format eq 'raw') {
 		run_command(['dd', "if=$file", "bs=4k", "status=progress"], output => '>&'.fileno($fh));
@@ -1603,20 +1604,19 @@ sub volume_export {
 	    return;
 	} elsif ($format =~ /^(qcow2|vmdk)\+size$/) {
 	    my $data_format = $1;
-	    goto unsupported if !$with_snapshots || $file_format ne $data_format;
+	    die $unsupported if !$with_snapshots || $file_format ne $data_format;
 	    write_common_header($fh, $size);
 	    run_command(['dd', "if=$file", "bs=4k", "status=progress"], output => '>&'.fileno($fh));
 	    return;
 	} elsif ($format eq 'tar+size') {
-	    goto unsupported if $file_format ne 'subvol';
+	    die $unsupported if $file_format ne 'subvol';
 	    write_common_header($fh, $size);
 	    run_command(['tar', @COMMON_TAR_FLAGS, '-cf', '-', '-C', $file, '.'],
 	                output => '>&'.fileno($fh));
 	    return;
 	}
     }
- unsupported:
-    die "volume export format $format not available for $class";
+    die $unsupported;
 }
 
 sub volume_export_formats {
-- 
2.39.2



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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [pve-devel] [PATCH storage] style: remove goto statements
  2024-06-26  8:56 [pve-devel] [PATCH storage] style: remove goto statements Fabian Grünbichler
@ 2024-06-27 13:28 ` Fiona Ebner
  2024-07-01  8:50   ` [pve-devel] applied: " Fabian Grünbichler
  0 siblings, 1 reply; 3+ messages in thread
From: Fiona Ebner @ 2024-06-27 13:28 UTC (permalink / raw)
  To: Proxmox VE development discussion, Fabian Grünbichler

Am 26.06.24 um 10:56 schrieb Fabian Grünbichler:
> these can just as well be `die` statements right there, there is no complicated
> cleanup that would warrant a goto statement..
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

but with some suggestions:

> ---
>  src/PVE/Storage/Plugin.pm | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index b5a54c1..f8dc9a2 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -1586,13 +1586,14 @@ sub read_common_header($) {
>  # Export a volume into a file handle as a stream of desired format.
>  sub volume_export {
>      my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots) = @_;

While at it, we could add a blank line here ;)

> +    my $unsupported = "volume export format $format not available for $class\n";

Maybe add _msg or _error or similar to improve the variable name?

>      if ($scfg->{path} && !defined($snapshot) && !defined($base_snapshot)) {
>  	my $file = $class->path($scfg, $volname, $storeid)
> -	    or goto unsupported;
> +	    or die $unsupported;

Could be moved to the previous line.

>  	my ($size, $file_format) = file_size_info($file);
>  
>  	if ($format eq 'raw+size') {
> -	    goto unsupported if $with_snapshots || $file_format eq 'subvol';
> +	    die $unsupported if $with_snapshots || $file_format eq 'subvol';
>  	    write_common_header($fh, $size);
>  	    if ($file_format eq 'raw') {
>  		run_command(['dd', "if=$file", "bs=4k", "status=progress"], output => '>&'.fileno($fh));


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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* [pve-devel] applied: [PATCH storage] style: remove goto statements
  2024-06-27 13:28 ` Fiona Ebner
@ 2024-07-01  8:50   ` Fabian Grünbichler
  0 siblings, 0 replies; 3+ messages in thread
From: Fabian Grünbichler @ 2024-07-01  8:50 UTC (permalink / raw)
  To: Fiona Ebner, Proxmox VE development discussion

applied with those suggestions folded in!

On June 27, 2024 3:28 pm, Fiona Ebner wrote:
> Am 26.06.24 um 10:56 schrieb Fabian Grünbichler:
>> these can just as well be `die` statements right there, there is no complicated
>> cleanup that would warrant a goto statement..
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> 
> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> 
> but with some suggestions:
> 
>> ---
>>  src/PVE/Storage/Plugin.pm | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>> 
>> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
>> index b5a54c1..f8dc9a2 100644
>> --- a/src/PVE/Storage/Plugin.pm
>> +++ b/src/PVE/Storage/Plugin.pm
>> @@ -1586,13 +1586,14 @@ sub read_common_header($) {
>>  # Export a volume into a file handle as a stream of desired format.
>>  sub volume_export {
>>      my ($class, $scfg, $storeid, $fh, $volname, $format, $snapshot, $base_snapshot, $with_snapshots) = @_;
> 
> While at it, we could add a blank line here ;)
> 
>> +    my $unsupported = "volume export format $format not available for $class\n";
> 
> Maybe add _msg or _error or similar to improve the variable name?
> 
>>      if ($scfg->{path} && !defined($snapshot) && !defined($base_snapshot)) {
>>  	my $file = $class->path($scfg, $volname, $storeid)
>> -	    or goto unsupported;
>> +	    or die $unsupported;
> 
> Could be moved to the previous line.
> 
>>  	my ($size, $file_format) = file_size_info($file);
>>  
>>  	if ($format eq 'raw+size') {
>> -	    goto unsupported if $with_snapshots || $file_format eq 'subvol';
>> +	    die $unsupported if $with_snapshots || $file_format eq 'subvol';
>>  	    write_common_header($fh, $size);
>>  	    if ($file_format eq 'raw') {
>>  		run_command(['dd', "if=$file", "bs=4k", "status=progress"], output => '>&'.fileno($fh));
> 


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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2024-07-01  8:51 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-06-26  8:56 [pve-devel] [PATCH storage] style: remove goto statements Fabian Grünbichler
2024-06-27 13:28 ` Fiona Ebner
2024-07-01  8:50   ` [pve-devel] applied: " Fabian Grünbichler

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal