Copy storage-ng XML / YAML devicegraph dump files to installation target system #694
Conversation
|
|
||
| log.info "copy log with '#{command}'" | ||
| def dest_dir | ||
| Yast::Installation.destdir + Yast::Directory.logdir |
There was a problem hiding this comment.
I'd prefer File.join(Yast::Installation.destdir, Yast::Directory.logdir) here as that is more robust (avoids multiple // or missing / in the result depending whether the trailing/beginning / is included or not).
There was a problem hiding this comment.
Since we do simplistic string operations here all over the place (as well as in so many other places in YaST), this would be inconsistent IMHO. As for the number of slashes (we had this same discussion in my last storage-ng PR only yesterday): Linux is robust about multiple ones; as a matter of fact, I cleaned up this code here just yesterday because it used to create paths like ///var/log/YaST2 all the time (and while it looks weird in the logs, it doesn't hurt at all). Yast::Directory.logdir is an absolute path, so we are safe against any missing slash here.
Sure, we can introduce File.join() in our code, but when we do it, please at least let's do it somewhat consistently and not only when somebody cleans up old messy code to have some mercy with people trying to figure out what happens here. And I really don't want to rewrite this whole thing completely (because that would be the consequence)...
| end | ||
|
|
||
| def copy_storage_inst_subdir | ||
| return if dest_dir == "/" |
There was a problem hiding this comment.
This looks a bit strange, dest_dir should never be / as it is destdir + logdir. Maybe Yast::Installation.destdir should be used here? That would make sense to avoid copying the files over themselves...
There was a problem hiding this comment.
You are right that it should not be /, but for some weird reason that happens. Maybe it's only in the unit tests (where this happens for sure). That made me think what will happen if this occurs also for real: This would already remove the entire directory that is to be copied. I hope that this would not happen in real life, but I'd rather be safe than sorry.
There was a problem hiding this comment.
But as you pointed above it might happen that it could be // or /// or ... which would not match here but Linux would treat is as /. That's why I suggested to use File.join which avoids that...
Maybe a little inconsistency is better in this case...
There was a problem hiding this comment.
OK, I'll change this one location. But we will have to make a general decision after a discussion about the pros and cons how we want to handle that in the future.
shundhammer
changed the title
|
|
||
| def copy_storage_inst_subdir | ||
| return if dest_dir == "/" | ||
| shell_cmd("/bin/rm -rf '#{dest_dir}/#{STORAGE_DUMP_DIR}'") |
There was a problem hiding this comment.
If we want to be consistent about using File.join(), this would also need to be changed. It would not be very readable anymore IMHO because this one-liner would then become two or three lines.
There was a problem hiding this comment.
These places are acceptable, I pointed out a place where it could make (ok, in some unusual situation) a problem.
| def copy_storage_inst_subdir | ||
| return if dest_dir == "/" | ||
| shell_cmd("/bin/rm -rf '#{dest_dir}/#{STORAGE_DUMP_DIR}'") | ||
| shell_cmd("/bin/cp -r '#{src_dir}/#{STORAGE_DUMP_DIR}' '#{dest_dir}/#{STORAGE_DUMP_DIR}'") |
There was a problem hiding this comment.
If we want to be consistent about using File.join(), this would also need to be changed. It would not be very readable anymore IMHO because this one-liner would then become two or three lines.
| dst_path = "#{Yast::Installation.destdir}/#{dir}/#{dst_file}" | ||
| command = "/bin/cp #{src_path} #{dst_path}" | ||
| def copy_log_to_target(src_file, dest_file = src_file) | ||
| shell_cmd("/bin/cp '#{src_dir}/#{src_file}' '#{dest_dir}/#{dest_file}'") |
There was a problem hiding this comment.
If we want to be consistent about using File.join(), this would also need to be changed. It would not be very readable anymore IMHO because this one-liner would then become two or three lines.
Also, compare with the old code; it used Ruby string expansion, but didn't even try to use File.join(). In the dst_path line it would even be a lot more justified to use File.join() because it's even 3 path components that are joined.
See also yast/yast-storage-ng#609
This copies
/var/log/YaST2/storage-instto the target at the end of the installation.Any old directory in the target system with that name is removed before so we don't get confused by upgrade after upgrade after upgrade when users submit y2logs with a bug report.
This is also a minor cleanup of that code; it had become quite messy over time.
Now all shell commands are properly logged, and there is less duplication in the code.