Cvd logs subcommand#2498
Conversation
ser-io
force-pushed
the
cvd-logs-subcommand
branch
from
346fc80 to
ff7c2bb
Compare
| Command cat("cat"); | ||
| cat.AddParameter(filename); | ||
| int exit_code = cat.Start().Wait(); | ||
| CF_EXPECTF(exit_code == 0, "{} {} failed: exited with status {}", | ||
| cat.Executable(), filename, exit_code); | ||
| return {}; |
There was a problem hiding this comment.
One of the suggestions in https://clig.dev/#output is
Use a pager (e.g.
less) if you are outputting a lot of text. For example,git diffdoes this by default. Using a pager can be error-prone, so be careful with your implementation such that you don’t make the experience worse for the user. Use a pager only ifstdinorstdoutis an interactive terminal.A good sensible set of options to use for
lessisless -FIRX. This does not page if the content fills one screen, ignores case when you search, enables color and formatting, and leaves the contents on the screen when less quits.
| CF_EXPECT(IsDirectory(logs_dir)); | ||
|
|
||
| if (opts.print_target.empty()) { | ||
| return PrintLogsList(logs_dir); |
There was a problem hiding this comment.
nit: return CF_EXPECT(PrintLogsList(logs_dir));
otherwise Handle() will not appear in the stack trace.
| } | ||
|
|
||
| func runCmdWithContextEnv(ctx context.Context, command []string, envvars map[string]string) (string, error) { | ||
| func runCmdWithContextEnv(ctx context.Context, command []string, envvars map[string]string) (string, string, error) { |
There was a problem hiding this comment.
Maybe worth returning a structure with fields for stdout and stderr so code isn't reliant on the caller knowing what each string means? It could be reused for RunCmdWithEnv and RunCmd as well.
|
|
||
| std::cout << "hello, logs\n"; | ||
| std::string dir = instance.instance_dir(); | ||
| std::string logs_dir = dir + "/logs"; |
There was a problem hiding this comment.
Some logs in the instance dir are symlinks to the assembly dir that may not be created if the instance fails to boot. This code should avoid those symlinks and go for the actual files instead.
There is also fetch.log in the artifacts dir.
There was a problem hiding this comment.
Good point. I noticed the following log names: assemble_cvd.log and cuttlefish_config.json and fetch.log are symlinks in the instance directory. Created b/509586405 to handle this in a different PR.
| std::string dir = instance.instance_dir(); | ||
| std::string logs_dir = dir + "/logs"; | ||
|
|
||
| CF_EXPECT(FileExists(logs_dir)); |
There was a problem hiding this comment.
cvd create --nostart && cvd logs is a valid command sequence but the second command will fail with an obscure error message about some directory not existing.
It should print a line informing the user that there are no logs available when no log files are found. If --verbose was given it could list the directories it searched too.
| GflagsCompatFlag("print", print_target), | ||
| GflagsCompatFlag("p", print_target), |
There was a problem hiding this comment.
This creates two different flags, instead of a single flag with two aliases. Do this instead:
GflagsCompatFlag("print", print_target).Alias({FlagAliasMode::kFlagConsumesFollowing, "-p"}),
| std::cout << std::endl; | ||
| return {}; | ||
| }; | ||
| return WalkDirectory(dir, callback); |
There was a problem hiding this comment.
Use CF_EXPECT on WalkDirectory so this function gets added to the stack trace.
| constexpr int kMaxPadding = 30; | ||
| std::string basename = android::base::Basename(filename); | ||
| std::cout << basename; | ||
| std::cout << std::string(std::max(int(kMaxPadding - basename.length()), 1), ' '); |
There was a problem hiding this comment.
nit: While the padding might make it look nice for a human it will complicate using this in a script. For example, cvd logs | cut -d" " -f2 can't be used to get the file paths because of the unpredictable number of spaces. You could use padding only if stdout is a terminal, like ls does to determine whether to print one file per line or not.
| constexpr int kMaxPadding = 30; | ||
| std::string basename = android::base::Basename(filename); | ||
| std::cout << basename; | ||
| std::cout << std::string(std::max(int(kMaxPadding - basename.length()), 1), ' '); |
There was a problem hiding this comment.
nit: Assign that expression to a variable to make it more clear what it is.
| } | ||
|
|
||
| Result<void> PrintLog(const std::string& filename) { | ||
| Command cat("cat"); |
There was a problem hiding this comment.
This adds an implicit dependency between cvd and cat. While it's unlikely cvd will run in a system that doesn't have cat it's still better to not depend on it. The simplest way to implement this in-process is something like:
std::ifstream file("/path/to/file");
CF_EXPECT(file.is_open());
std::cout << file.rdbuf();
If you still think calling cat is preferable for some reason then do a plain exec without fork to at least avoid creating a new process unnecessarily.
| } | ||
| listedNames = append(listedNames, fields[0]) | ||
| } | ||
| keyExpectedNames := []string{"logcat", "launcher.log", "kernel.log"} |
There was a problem hiding this comment.
also assemble_cvd.log and fetch.log when launched with cvd load.
Bug: b/507588992
Bug: b/507588992
Bug: b/507588992
Bug: b/507588992
Bug: b/507588992
Bug: b/507588992
ser-io
force-pushed
the
cvd-logs-subcommand
branch
from
ff7c2bb to
1177b9c
Compare
3 participants
No description provided.