[pve-devel] [PATCH backup/pmg-api/manager/common v4] fix: #3971 Tasklog download button

[pve-devel] [PATCH backup/pmg-api/manager/common v4] fix: #3971 Tasklog download button

[Daniel Tschlatscher]

This patch series' aim is to add a download button in the tasklog-
viewer GUI so that users may access the tasklog more easily.
(The tasklog-viewer only displays 50 lines at a time)
Instead of suddenly returning a file stream when the 'limit' parameter
is set to 0, now, a new parameter 'download' needs to be passed.
This parameter is mutually exclusive with the other parameters.

Before, the backup server backend would simply return an array with no
content for a call with parameter 'limit=0', the pmg and pve backends
would fall back to the default of 50. To unify this, setting 'limit=0'
will now return everything until the end of the log file in the normal
json format. This is useful in case it is unknown how many lines the
log contains, with 0 being a bit nonsensical for other cases anyway.
If no parameter is given, the default of 50 still applies.

=> Thanks to Fabian for the review!

changes from v3
* parameter 'download' for downloading the task log
* limit=0 now returns the whole log instead
* miscellaneous code clean up


proxmox-backup:

Daniel Tschlatscher (1):
  make tasklog downloadable in the backup server backend

 src/api2/node/tasks.rs | 209 +++++++++++++++++++++++++----------------
 1 file changed, 129 insertions(+), 80 deletions(-)

pmg-api:

Daniel Tschlatscher (1):
  make tasklog downloadable in the PMG backend

 src/PMG/API2/Tasks.pm | 60 ++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 24 deletions(-)

pve-manager:

Daniel Tschlatscher (1):
  make task log downloadable in the PVE manager backend

 PVE/API2/Tasks.pm | 48 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 7 deletions(-)

pve-common:

Daniel Tschlatscher (1):
  return whole log file if limit is 0

 src/PVE/Tools.pm | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH backup v4] make tasklog downloadable in the backup server backend

[Daniel Tschlatscher]

The read_tasklog API call now stream the whole log file if the query
parameter 'download' is set to true. If the limit parameter is set to
0, all lines in the tasklog will be returned in json format.

To make a file stream and a json response in the same API call work, I
had to use one of the lower level apimethod types from the
proxmox-router. Therefore, the routing declarations and parameter
schemas have been changed accordingly.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
I sent this separately, as the other 3 patches in this version do not
concern the backup patch in any way.

Changes from v3:
* API parameter 'download' for streaming the file directly
* option 'limit=0' returns the whole task log
* read_task_log_lines() function is inline in the code again as it's
  the only occurrence and doesn't add that much readability

 src/api2/node/tasks.rs | 209 +++++++++++++++++++++++++----------------
 1 file changed, 129 insertions(+), 80 deletions(-)

diff --git a/src/api2/node/tasks.rs b/src/api2/node/tasks.rs
index cbd0894e..258c3e86 100644
--- a/src/api2/node/tasks.rs
+++ b/src/api2/node/tasks.rs
@@ -2,10 +2,18 @@ use std::fs::File;
 use std::io::{BufRead, BufReader};
 
 use anyhow::{bail, Error};
+use futures::FutureExt;
+use http::request::Parts;
+use http::{header, Response, StatusCode};
+use hyper::Body;
+use proxmox_async::stream::AsyncReaderStream;
 use serde_json::{json, Value};
 
-use proxmox_router::{list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap};
-use proxmox_schema::api;
+use proxmox_router::{
+    list_subdirs_api_method, ApiHandler, ApiMethod, ApiResponseFuture, Permission, Router,
+    RpcEnvironment, SubdirMap,
+};
+use proxmox_schema::{api, BooleanSchema, IntegerSchema, ObjectSchema, Schema};
 use proxmox_sys::sortable;
 
 use pbs_api_types::{
@@ -19,6 +27,27 @@ use crate::api2::pull::check_pull_privs;
 use pbs_config::CachedUserInfo;
 use proxmox_rest_server::{upid_log_path, upid_read_status, TaskListInfoIterator, TaskState};
 
+pub const START_PARAM_SCHEMA: Schema =
+    IntegerSchema::new("Start at this line when reading the tasklog")
+        .minimum(0)
+        .default(0)
+        .schema();
+
+pub const LIMIT_PARAM_SCHEMA: Schema =
+    IntegerSchema::new("The amount of lines to read from the tasklog. Setting this parameter to 0 will return all lines until the end of the file.")
+        .minimum(0)
+        .default(50)
+        .schema();
+
+pub const DOWNLOAD_PARAM_SCHEMA: Schema =
+    BooleanSchema::new("Whether the tasklog file should be downloaded. This parameter can't be used in conjunction with other parameters")
+        .default(false)
+        .schema();
+
+pub const TEST_STATUS_PARAM_SCHEMA: Schema =
+    BooleanSchema::new("Test task status, and set result attribute \"active\" accordingly.")
+        .schema();
+
 // matches respective job execution privileges
 fn check_job_privs(auth_id: &Authid, user_info: &CachedUserInfo, upid: &UPID) -> Result<(), Error> {
     match (upid.worker_type.as_str(), &upid.worker_id) {
@@ -268,90 +297,110 @@ fn extract_upid(param: &Value) -> Result<UPID, Error> {
     upid_str.parse::<UPID>()
 }
 
-#[api(
-    input: {
-        properties: {
-            node: {
-                schema: NODE_SCHEMA,
-            },
-            upid: {
-                schema: UPID_SCHEMA,
-            },
-            "test-status": {
-                type: bool,
-                optional: true,
-                description: "Test task status, and set result attribute \"active\" accordingly.",
-            },
-            start: {
-                type: u64,
-                optional: true,
-                description: "Start at this line.",
-                default: 0,
-            },
-            limit: {
-                type: u64,
-                optional: true,
-                description: "Only list this amount of lines.",
-                default: 50,
-            },
-        },
-    },
-    access: {
-        description: "Users can access their own tasks, or need Sys.Audit on /system/tasks.",
-        permission: &Permission::Anybody,
-    },
-)]
-/// Read task log.
-async fn read_task_log(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
-    let upid = extract_upid(&param)?;
-
-    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
-
-    check_task_access(&auth_id, &upid)?;
-
-    let test_status = param["test-status"].as_bool().unwrap_or(false);
-
-    let start = param["start"].as_u64().unwrap_or(0);
-    let mut limit = param["limit"].as_u64().unwrap_or(50);
-
-    let mut count: u64 = 0;
-
-    let path = upid_log_path(&upid)?;
-
-    let file = File::open(path)?;
-
-    let mut lines: Vec<Value> = vec![];
+#[sortable]
+pub const API_METHOD_READ_TASK_LOG: ApiMethod = ApiMethod::new(
+    &ApiHandler::AsyncHttp(&read_task_log),
+    &ObjectSchema::new(
+        "Read the task log",
+        &sorted!([
+            ("node", false, &NODE_SCHEMA),
+            ("upid", false, &UPID_SCHEMA),
+            ("start", true, &START_PARAM_SCHEMA),
+            ("limit", true, &LIMIT_PARAM_SCHEMA),
+            ("download", true, &DOWNLOAD_PARAM_SCHEMA),
+            ("test-status", true, &TEST_STATUS_PARAM_SCHEMA)
+        ]),
+    ),
+)
+.access(
+    Some("Users can access their own tasks, or need Sys.Audit on /system/tasks."),
+    &Permission::Anybody,
+);
+fn read_task_log(
+    _parts: Parts,
+    _req_body: Body,
+    param: Value,
+    _info: &ApiMethod,
+    rpcenv: Box<dyn RpcEnvironment>,
+) -> ApiResponseFuture {
+    async move {
+        let upid: UPID = extract_upid(&param)?;
+        let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+        check_task_access(&auth_id, &upid)?;
+
+        let download = param["download"].as_bool().unwrap_or(false);
+        let path = upid_log_path(&upid)?;
+
+        if download {
+            if !param["start"].is_null()
+                || !param["limit"].is_null()
+                || !param["test-status"].is_null()
+            {
+                bail!("Parameter 'download' cannot be used with other parameters");
+            }
 
-    for line in BufReader::new(file).lines() {
-        match line {
-            Ok(line) => {
-                count += 1;
-                if count < start {
-                    continue;
-                };
-                if limit == 0 {
-                    continue;
-                };
+            let header_disp = format!("attachment; filename={}", &upid.to_string());
+            let stream = AsyncReaderStream::new(tokio::fs::File::open(path).await?);
+
+            Ok(Response::builder()
+                .status(StatusCode::OK)
+                .header(header::CONTENT_TYPE, "text/plain")
+                .header(header::CONTENT_DISPOSITION, &header_disp)
+                .body(Body::wrap_stream(stream))
+                .unwrap())
+        } else {
+            let start = param["start"].as_u64().unwrap_or(0);
+            let mut limit = param["limit"].as_u64().unwrap_or(50);
+            let test_status = param["test-status"].as_bool().unwrap_or(false);
+
+            let file = File::open(path)?;
+
+            let mut count: u64 = 0;
+            let mut lines: Vec<Value> = vec![];
+            let read_until_end = if limit == 0 { true } else { false };
+
+            for line in BufReader::new(file).lines() {
+                match line {
+                    Ok(line) => {
+                        count += 1;
+                        if count < start {
+                            continue;
+                        };
+                        if !read_until_end {
+                            if limit == 0 {
+                                continue;
+                            };
+                            limit -= 1;
+                        }
+
+                        lines.push(json!({ "n": count, "t": line }));
+                    }
+                    Err(err) => {
+                        log::error!("reading task log failed: {}", err);
+                        break;
+                    }
+                }
+            }
 
-                lines.push(json!({ "n": count, "t": line }));
+            let mut json = json!({
+                "data": lines,
+                "total": count,
+                "success": 1,
+            });
 
-                limit -= 1;
+            if test_status {
+                let active = proxmox_rest_server::worker_is_active(&upid).await?;
+                json["test-status"] = Value::from(active);
             }
-            Err(err) => {
-                log::error!("reading task log failed: {}", err);
-                break;
-            }
-        }
-    }
 
-    rpcenv["total"] = Value::from(count);
-
-    if test_status {
-        let active = proxmox_rest_server::worker_is_active(&upid).await?;
-        rpcenv["active"] = Value::from(active);
+            Ok(Response::builder()
+                .status(StatusCode::OK)
+                .header(header::CONTENT_TYPE, "application/json")
+                .body(Body::from(json.to_string()))
+                .unwrap())
+        }
     }
-
-    Ok(json!(lines))
+    .boxed()
 }
 
 #[api(
-- 
2.30.2

[pve-devel] applied: [pbs-devel] [PATCH backup v4] make tasklog downloadable in the backup server backend

[Wolfgang Bumiller]

applied with a cleanup followup commit, see below

On Wed, Nov 23, 2022 at 03:52:07PM +0100, Daniel Tschlatscher wrote:
> The read_tasklog API call now stream the whole log file if the query
> parameter 'download' is set to true. If the limit parameter is set to
> 0, all lines in the tasklog will be returned in json format.
> 
> To make a file stream and a json response in the same API call work, I
> had to use one of the lower level apimethod types from the
> proxmox-router. Therefore, the routing declarations and parameter
> schemas have been changed accordingly.
> 
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
> ---
> I sent this separately, as the other 3 patches in this version do not
> concern the backup patch in any way.
> 
> Changes from v3:
> * API parameter 'download' for streaming the file directly
> * option 'limit=0' returns the whole task log
> * read_task_log_lines() function is inline in the code again as it's
>   the only occurrence and doesn't add that much readability
> 
>  src/api2/node/tasks.rs | 209 +++++++++++++++++++++++++----------------
>  1 file changed, 129 insertions(+), 80 deletions(-)
> 
> diff --git a/src/api2/node/tasks.rs b/src/api2/node/tasks.rs
> index cbd0894e..258c3e86 100644
> --- a/src/api2/node/tasks.rs
> +++ b/src/api2/node/tasks.rs
> @@ -2,10 +2,18 @@ use std::fs::File;
>  use std::io::{BufRead, BufReader};
>  
>  use anyhow::{bail, Error};
> +use futures::FutureExt;
> +use http::request::Parts;
> +use http::{header, Response, StatusCode};
> +use hyper::Body;
> +use proxmox_async::stream::AsyncReaderStream;
>  use serde_json::{json, Value};
>  
> -use proxmox_router::{list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap};
> -use proxmox_schema::api;
> +use proxmox_router::{
> +    list_subdirs_api_method, ApiHandler, ApiMethod, ApiResponseFuture, Permission, Router,
> +    RpcEnvironment, SubdirMap,
> +};
> +use proxmox_schema::{api, BooleanSchema, IntegerSchema, ObjectSchema, Schema};
>  use proxmox_sys::sortable;
>  
>  use pbs_api_types::{
> @@ -19,6 +27,27 @@ use crate::api2::pull::check_pull_privs;
>  use pbs_config::CachedUserInfo;
>  use proxmox_rest_server::{upid_log_path, upid_read_status, TaskListInfoIterator, TaskState};
>  
> +pub const START_PARAM_SCHEMA: Schema =
> +    IntegerSchema::new("Start at this line when reading the tasklog")
> +        .minimum(0)
> +        .default(0)
> +        .schema();
> +
> +pub const LIMIT_PARAM_SCHEMA: Schema =
> +    IntegerSchema::new("The amount of lines to read from the tasklog. Setting this parameter to 0 will return all lines until the end of the file.")

^ Please split lines longer than 100 chars, like:

    IntegerSchema::new(
        "The amount of lines to read from the tasklog. \
         Setting this parameter to 0 will return all lines until the end of the file."
    )


(...)
> +        if download {
> +            if !param["start"].is_null()
> +                || !param["limit"].is_null()
> +                || !param["test-status"].is_null()
> +            {
> +                bail!("Parameter 'download' cannot be used with other parameters");
> +            }
>  
> -    for line in BufReader::new(file).lines() {
> -        match line {
> -            Ok(line) => {
> -                count += 1;
> -                if count < start {
> -                    continue;
> -                };
> -                if limit == 0 {
> -                    continue;
> -                };
> +            let header_disp = format!("attachment; filename={}", &upid.to_string());
> +            let stream = AsyncReaderStream::new(tokio::fs::File::open(path).await?);
> +
> +            Ok(Response::builder()
> +                .status(StatusCode::OK)
> +                .header(header::CONTENT_TYPE, "text/plain")
> +                .header(header::CONTENT_DISPOSITION, &header_disp)
> +                .body(Body::wrap_stream(stream))
> +                .unwrap())

^ This could just have a `return` in front of it, so the remaining code
isn't that much indented.

> +        } else {
> +            let start = param["start"].as_u64().unwrap_or(0);
> +            let mut limit = param["limit"].as_u64().unwrap_or(50);
> +            let test_status = param["test-status"].as_bool().unwrap_or(false);
> +
> +            let file = File::open(path)?;
> +
> +            let mut count: u64 = 0;
> +            let mut lines: Vec<Value> = vec![];
> +            let read_until_end = if limit == 0 { true } else { false };

^ booleans are already booleans, please avoid this construct in the
future.

clippy can catch this btw.

[pve-devel] [PATCH pmg-api v4] make tasklog downloadable in the PMG backend

[Daniel Tschlatscher]

The read_tasklog API call now stream the whole log file if the query
parameter 'download' is set to true.
This is done in preparation for the task log download button to be
added in the TaskViewer.

I saw an opportunity here to clear some redundant code for displaying
the tasklog and replaced it with a call to dump_logfile(), akin to how
this is handled in pve-manager.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
Changes from v3:
* API parameter 'download' for directly streaming the file
* Added missing API parameter descriptions

 src/PMG/API2/Tasks.pm | 60 ++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/src/PMG/API2/Tasks.pm b/src/PMG/API2/Tasks.pm
index 598fb4d..5a6f002 100644
--- a/src/PMG/API2/Tasks.pm
+++ b/src/PMG/API2/Tasks.pm
@@ -238,12 +238,19 @@ __PACKAGE__->register_method({
 		type => 'integer',
 		minimum => 0,
 		optional => 1,
+		description => "Start at this line when reading the tasklog",
 	    },
 	    limit => {
 		type => 'integer',
 		minimum => 0,
 		optional => 1,
+		description => "The amount of lines to read from the tasklog.",
 	    },
+	    download => {
+		type => 'boolean',
+		optional => 1,
+		description => "Whether the tasklog file should be downloaded. This parameter can't be used in conjunction with other parameters",
+	    }
 	},
     },
     returns => {
@@ -268,37 +275,42 @@ __PACKAGE__->register_method({
 	my ($task, $filename) = PVE::Tools::upid_decode($param->{upid}, 1);
 	raise_param_exc({ upid => "unable to parse worker upid" }) if !$task;
 
-	my $lines = [];
+	my $download = $param->{download} // 0;
 
-	my $restenv = PMG::RESTEnvironment->get();
+	if ($download) {
+	    die "Parameter 'download' can't be used with other parameters\n"
+		if (defined($param->{start}) || defined($param->{limit}));
 
-	my $fh = IO::File->new($filename, "r");
-	raise_param_exc({ upid => "no such task - unable to open file - $!" }) if !$fh;
+	    my $fh;
+	    my $use_compression = ( -s $filename ) > 1024;
 
-	my $start = $param->{start} || 0;
-	my $limit = $param->{limit} || 50;
-	my $count = 0;
-	my $line;
-	while (defined ($line = <$fh>)) {
-	    next if $count++ < $start;
-	    next if $limit <= 0;
-	    chomp $line;
-	    push @$lines, { n => $count, t => $line};
-	    $limit--;
-	}
+	    # 1024 is a practical cutoff for the size distribution of our log files.
+	    if ($use_compression) {
+		open($fh, "-|", "/usr/bin/gzip", "-c", "$filename")
+		    or die "Could not create compressed file stream for file '$filename' - $!\n";
+	    } else {
+		open($fh, '<', $filename) or die "Could not open file '$filename' - $!\n";
+	    }
 
-	close($fh);
+	    return {
+		download => {
+		    fh => $fh,
+		    stream => 1,
+		    'content-encoding' => $use_compression ? 'gzip' : undef,
+		    'content-type' => "text/plain",
+		    'content-disposition' => "attachment; filename=\"".$param->{upid}."\"",
+		},
+	    },
+	} else {
+	    my $start = $param->{start} // 0;
+	    my $limit = $param->{limit} // 50;
 
-	# HACK: ExtJS store.guaranteeRange() does not like empty array
-	# so we add a line
-	if (!$count) {
-	    $count++;
-	    push @$lines, { n => $count, t => "no content"};
-	}
+	    my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
 
-	$restenv->set_result_attrib('total', $count);
+	    PMG::RESTEnvironment->get()->set_result_attrib('total', $count);
 
-	return $lines;
+	    return $lines;
+	}
     }});
 
 
-- 
2.30.2

[pve-devel] [PATCH manager v4] make task log downloadable in the PVE manager backend

[Daniel Tschlatscher]

The read_tasklog API call now stream the whole log file if the query
parameter 'download' is set to true.
This is done in preparation for the task log download button to be
added in the TaskViewer.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
Changes from v3:
* API parameter 'download' for directly streaming the file
* Adapted API parameter descriptions to other backends

 PVE/API2/Tasks.pm | 48 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 7 deletions(-)

diff --git a/PVE/API2/Tasks.pm b/PVE/API2/Tasks.pm
index 9cd1e56b..16041720 100644
--- a/PVE/API2/Tasks.pm
+++ b/PVE/API2/Tasks.pm
@@ -342,15 +342,20 @@ __PACKAGE__->register_method({
 		minimum => 0,
 		default => 0,
 		optional => 1,
-		description => "The line number to start printing at.",
+		description => "Start at this line when reading the tasklog",
 	    },
 	    limit => {
 		type => 'integer',
 		minimum => 0,
 		default => 50,
 		optional => 1,
-		description => "The maximum amount of lines that should be printed.",
+		description => "The amount of lines to read from the tasklog.",
 	    },
+	    download => {
+		type => 'boolean',
+		optional => 1,
+		description => "Whether the tasklog file should be downloaded. This parameter can't be used in conjunction with other parameters",
+	    }
 	},
     },
     returns => {
@@ -378,8 +383,6 @@ __PACKAGE__->register_method({
 	my $rpcenv = PVE::RPCEnvironment::get();
 	my $user = $rpcenv->get_user();
 	my $node = $param->{node};
-	my $start = $param->{start} // 0;
-	my $limit = $param->{limit} // 50;
 
 	$convert_token_task->($task);
 
@@ -387,11 +390,42 @@ __PACKAGE__->register_method({
 	    $rpcenv->check($user, "/nodes/$node", [ 'Sys.Audit' ]);
 	}
 
-	my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
+	my $download = $param->{download} // 0;
 
-	$rpcenv->set_result_attrib('total', $count);
+	if ($download) {
+	    die "Parameter 'download' can't be used with other parameters\n"
+		if (defined($param->{start}) || defined($param->{limit}));
 
-	return $lines;
+	    my $fh;
+	    my $use_compression = ( -s $filename ) > 1024;
+
+	    # 1024 is a practical cutoff for the size distribution of our log files.
+	    if ($use_compression) {
+		open($fh, "-|", "/usr/bin/gzip", "-c", "$filename")
+		    or die "Could not create compressed file stream for file '$filename' - $!\n";
+	    } else {
+		open($fh, '<', $filename) or die "Could not open file '$filename' - $!\n";
+	    }
+
+	    return {
+		download => {
+		    fh => $fh,
+		    stream => 1,
+		    'content-encoding' => $use_compression ? 'gzip' : undef,
+		    'content-type' => "text/plain",
+		    'content-disposition' => "attachment; filename=\"".$param->{upid}."\"",
+		},
+	    },
+	} else {
+	    my $start = $param->{start} // 0;
+	    my $limit = $param->{limit} // 50;
+
+	    my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
+
+	    $rpcenv->set_result_attrib('total', $count);
+
+	    return $lines;
+	}
     }});
 
 
-- 
2.30.2

[pve-devel] [PATCH common v4] return whole log file if limit is 0

[Daniel Tschlatscher]

The dump_logfile now returns the whole log file if the limit parameter
is set to 0. This must be done explicitely though, as in the case of
'limit' being undefined, the default as before, 50 will be used.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
Changes from v3:
* NEW patch

This patch also affects the API endpoints for:
* the ceph log file
* replication log files
* firewall Guest and Host log files

It does not change how they behave in the GUI though, as all of the
corresponding API calls there do not specify a 'limit=0' parameter.
In fact, none of them specify any limit, therefore, the default value
of 50 still applies.

 src/PVE/Tools.pm | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index eb81b96..6072eb7 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -1278,9 +1278,10 @@ sub dump_logfile {
 	return ($count, $lines);
     }
 
-    $start = 0 if !$start;
-    $limit = 50 if !$limit;
+    $start = $start // 0;
+    $limit = $limit // 50;
 
+    my $read_until_end = ($limit == 0) ? 1 : 0;
     my $line;
 
     if ($filter) {
@@ -1288,18 +1289,22 @@ sub dump_logfile {
 	while (defined($line = <$fh>)) {
 	    next if $line !~ m/$filter/;
 	    next if $count++ < $start;
-	    next if $limit <= 0;
+	    if (!$read_until_end) {
+		next if $limit <= 0;
+		$limit--;
+	    }
 	    chomp $line;
 	    push @$lines, { n => $count, t => $line};
-	    $limit--;
 	}
     } else {
 	while (defined($line = <$fh>)) {
 	    next if $count++ < $start;
-	    next if $limit <= 0;
+	    if (!$read_until_end) {
+		next if $limit <= 0;
+		$limit--;
+	    }
 	    chomp $line;
 	    push @$lines, { n => $count, t => $line};
-	    $limit--;
 	}
     }
 
-- 
2.30.2

[pve-devel] applied: [PATCH common v4] return whole log file if limit is 0

[Thomas Lamprecht]

Am 23/11/2022 um 15:52 schrieb Daniel Tschlatscher:
> The dump_logfile now returns the whole log file if the limit parameter
> is set to 0. This must be done explicitely though, as in the case of
> 'limit' being undefined, the default as before, 50 will be used.
> 
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
> ---
> Changes from v3:
> * NEW patch
> 
> This patch also affects the API endpoints for:
> * the ceph log file
> * replication log files
> * firewall Guest and Host log files
> 
> It does not change how they behave in the GUI though, as all of the
> corresponding API calls there do not specify a 'limit=0' parameter.
> In fact, none of them specify any limit, therefore, the default value
> of 50 still applies.
> 
>  src/PVE/Tools.pm | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)


applied, thanks!

> 
> diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
> index eb81b96..6072eb7 100644
> --- a/src/PVE/Tools.pm
> +++ b/src/PVE/Tools.pm
> @@ -1278,9 +1278,10 @@ sub dump_logfile {
>  	return ($count, $lines);
>      }
>  
> -    $start = 0 if !$start;
> -    $limit = 50 if !$limit;
> +    $start = $start // 0;
> +    $limit = $limit // 50;
>  
> +    my $read_until_end = ($limit == 0) ? 1 : 0;

no need for transform booleans to booleans ;-)