Added filename placeholder, execute print command as lp, fixed memory leaks (#677)

* Added filename placeholder

* Comment update

* Execute print command as lp user

* Comment update

* Comment update

* Added operaton parameters

* Sort parameter output by name

* Removed assertion

* Revert "Removed assertion"

This reverts commit f9ab582ddc.

* Include cleanup

* Code cleanup

* Updated SCSI reset

* Fixed memory leak

* Revert "Fixed memory leak"

This reverts commit 2dbbdcd192.

* Fixed memory leak

* Fixed memory leak

* Updated operation count check

* Fixed memory leaks

* Delete temporary PbOperationInfo

src/raspberrypi/controllers/sasidev_ctrl.h

@@ -19,7 +19,6 @@
 #include "os.h"
 #include "scsi.h"
 #include "fileio.h"
-#include "log.h"
 
 class Device;
 

src/raspberrypi/controllers/scsidev_ctrl.cpp

@@ -45,13 +45,14 @@ SCSIDEV::~SCSIDEV()
 
 void SCSIDEV::Reset()
 {
+	scsi.bytes_to_transfer = 0;
+
 	// Work initialization
 	scsi.atnmsg = false;
 	scsi.msc = 0;
 	memset(scsi.msb, 0x00, sizeof(scsi.msb));
 
-	// Base class
-	SASIDEV::Reset();
+	super::Reset();
 }
 
 BUS::phase_t SCSIDEV::Process(int initiator_id)
@@ -891,7 +892,7 @@ void SCSIDEV::ReceiveBytes()
 bool SCSIDEV::XferOut(bool cont)
 {
 	if (!scsi.is_byte_transfer) {
-		return SASIDEV::XferOut(cont);
+		return super::XferOut(cont);
 	}
 
 	ASSERT(ctrl.phase == BUS::dataout);

src/raspberrypi/controllers/scsidev_ctrl.h

@@ -77,6 +77,7 @@ public:
 	void SetByteTransfer(bool is_byte_transfer) { scsi.is_byte_transfer = is_byte_transfer; }
 
 private:
+	typedef SASIDEV super;
 
 	// Phases
 	void BusFree() override;

src/raspberrypi/devices/device_factory.cpp

@@ -53,7 +53,7 @@ DeviceFactory::DeviceFactory()
 
 	default_params[SCBR]["interfaces"] = network_interfaces;
 	default_params[SCDP]["interfaces"] = network_interfaces;
-	default_params[SCLP]["cmd"] = "lp -oraw";
+	default_params[SCLP]["cmd"] = "lp -oraw %f";
 	default_params[SCLP]["timeout"] = "30";
 
 	extension_mapping["hdf"] = SAHD;

src/raspberrypi/devices/scsi_printer.cpp

@@ -25,10 +25,12 @@
 // always using a reservation is recommended.
 //
 // The command to be used for printing can be set with the "cmd" property when attaching the device.
-// By default the data to be printed are sent to the printer unmodified, using "lp -oraw". This either
+// By default the data to be printed are sent to the printer unmodified, using "lp -oraw %f". This
 // requires that the client uses a printer driver compatible with the respective printer, or that the
-// printing service on the Pi is configured to do any necessary conversions.
+// printing service on the Pi is configured to do any necessary conversions, or that the print command
+// applies any conversions on the file to be printed (%f) before passing it to the printing service.
 // By attaching different devices/LUNs multiple printers (i.e. different print commands) are possible.
+// Note that the print command is not executed by root but with the permissions of the lp user.
 //
 // With STOP PRINT printing can be cancelled before SYNCHRONIZE BUFFER was sent.
 //
@@ -73,7 +75,13 @@ bool SCSIPrinter::Init(const map<string, string>& params)
 	// Use default parameters if no parameters were provided
 	SetParams(params.empty() ? GetDefaultParams() : params);
 
+	if (GetParam("cmd").find("%f") == string::npos) {
+		LOGERROR("Missing filename specifier %s", "%f");
+		return false;
+	}
+
 	if (!GetAsInt(GetParam("timeout"), timeout) || timeout <= 0) {
+		LOGERROR("Reservation timeout value must be > 0");
 		return false;
 	}
 
@@ -192,8 +200,10 @@ void SCSIPrinter::SynchronizeBuffer(SCSIDEV *controller)
 	fd = -1;
 
 	string cmd = GetParam("cmd");
-	cmd += " ";
-	cmd += filename;
+	size_t file_position = cmd.find("%f");
+	assert(file_position != string::npos);
+	cmd.replace(file_position, 2, filename);
+	cmd = "sudo -u lp " + cmd;
 
 	LOGTRACE("%s", string("Printing file with size of " + to_string(st.st_size) +" byte(s)").c_str());
 

src/raspberrypi/rascsi.cpp

@@ -829,6 +829,8 @@ void TerminationHandler(int signum)
 {
 	DetachAll();
 
+	Cleanup();
+
 	exit(signum);
 }
 
@@ -1591,9 +1593,13 @@ int main(int argc, char* argv[])
 {
 	GOOGLE_PROTOBUF_VERIFY_VERSION;
 
+#ifndef NDEBUG
 	// Get temporary operation info, in order to trigger an assertion on startup if the operation list is incomplete
 	PbResult pb_operation_info_result;
-	rascsi_response.GetOperationInfo(pb_operation_info_result, 0);
+	const PbOperationInfo *operation_info = rascsi_response.GetOperationInfo(pb_operation_info_result, 0);
+	assert(operation_info->operations_size() == PbOperation_ARRAYSIZE - 1);
+	delete operation_info;
+#endif
 
 	int actid;
 	BUS::phase_t phase;

src/raspberrypi/rascsi_interface.proto

@@ -42,7 +42,7 @@ enum PbOperation {
     // Parameters (depending on the device type):
     //   "file": The filename relative to the default image folder
     //   "interfaces": A prioritized comma-separated list of interfaces to create a network bridge for
-    //   "cmd": The command to be used for printing
+    //   "cmd": The command to be used for printing, with "%f" as file placeholder
     //   "timeout": The timeout for printer reservations in seconds
     ATTACH = 1;
 

src/raspberrypi/rascsi_response.cpp

@@ -370,6 +370,8 @@ PbOperationInfo *RascsiResponse::GetOperationInfo(PbResult& result, int depth)
 	PbOperationMetaData *meta_data = new PbOperationMetaData();
 	AddOperationParameter(meta_data, "name", "Image file name in case of a mass storage device");
 	AddOperationParameter(meta_data, "interfaces", "Comma-separated prioritized network interface list");
+	AddOperationParameter(meta_data, "cmd", "print command for the printer device");
+	AddOperationParameter(meta_data, "timeout", "Reservation timeout for the printer device in seconds");
 	CreateOperation(operation_info, meta_data, ATTACH, "Attach device, device-specific parameters are required");
 
 	meta_data = new PbOperationMetaData();
@@ -498,9 +500,6 @@ PbOperationInfo *RascsiResponse::GetOperationInfo(PbResult& result, int depth)
 	meta_data = new PbOperationMetaData();
 	CreateOperation(operation_info, meta_data, OPERATION_INFO, "Get operation meta data");
 
-	// Ensure that the complete set of operations is covered
-	assert(operation_info->operations_size() == PbOperation_ARRAYSIZE - 1);
-
 	result.set_status(true);
 
 	return operation_info;
@@ -513,6 +512,7 @@ void RascsiResponse::CreateOperation(PbOperationInfo *operation_info, PbOperatio
 	meta_data->set_description(description);
 	int ordinal = PbOperation_descriptor()->FindValueByName(PbOperation_Name(operation))->index();
 	(*operation_info->mutable_operations())[ordinal] = *meta_data;
+	delete meta_data;
 }
 
 PbOperationParameter *RascsiResponse::AddOperationParameter(PbOperationMetaData *meta_data, const string& name,

src/raspberrypi/rasctl_display.cpp

@@ -304,7 +304,10 @@ void RasctlDisplay::DisplayOperationInfo(const PbOperationInfo& operation_info)
 			}
 			cout << endl;
 
-			for (const auto& parameter : operation.second.parameters()) {
+			list<PbOperationParameter> sorted_parameters = { operation.second.parameters().begin(), operation.second.parameters().end() };
+			sorted_parameters.sort([](const auto& a, const auto& b) { return a.name() < b.name(); });
+
+			for (const auto& parameter : sorted_parameters) {
 				cout << "    " << parameter.name() << ": "
 					<< (parameter.is_mandatory() ? "mandatory" : "optional");
 				if (!parameter.description().empty()) {