Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Say hello to Streamer (tape) device support #1477

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

bog-dan-ro
Copy link

@bog-dan-ro bog-dan-ro commented Aug 22, 2024

Implements the mandatory and most of the optional commands for tandberd see

https://bitsavers.org/pdf/tandbergData/TDC4100/6047-1_TDC-4100_SCSI-2_Interface_Functional_Specification_Aug1991.pdf for more info.

Fixed #480

I tested this code on my Altos 386 Series 1000. Read, Write, Rewind, ModeSense/Select, etc. operations worked fine.

Also, I didn't used the caching mecanism from Disk as the streamers are sequential and there is no chance to read/write the same block more than once in a very short period of time.

Personally I wanted to use the posix file api, but I thought it might be way too low level ;-)

@akuker
Copy link
Member

akuker commented Aug 23, 2024

Thank you for the pull request!! Could you please take a look at the sonarcloud issues that were reported?

@rdmark
Copy link
Member

rdmark commented Aug 23, 2024

Good stuff! The python linter reports a minor issue. You can fix it automatically by running black. Please see https://github.com/PiSCSI/piscsi/blob/develop/python/README.md#static-analysis-and-formatting

@rdmark
Copy link
Member

rdmark commented Aug 23, 2024

Also, you have 20 failed python tests, typically indicative of tests needing to be adjusted for the new feature.

@bog-dan-ro
Copy link
Author

bog-dan-ro commented Aug 23, 2024

Also, you have 20 failed python tests, typically indicative of tests needing to be adjusted for the new feature.

Regarding that, there is another issue besides the tests.

The web support for streamer is far from ideal ...:

  • I could not managed link the tape files extensions (.tar, .tap) to the SCST (scisi streamer)
  • I could not managed to add a list of common tape sizes for uses to create new images
  • After I eject the current streamer file, I can not pick a new one, the drop down combobox is empty.

Sadly my python and HTML skills are closed to zero, I can do small changes by instinct but no more than that.
So, if anyone can help in this area, it will be highly appreciated!

@bog-dan-ro
Copy link
Author

Thank you for the pull request!! Could you please take a look at the sonarcloud issues that were reported?

I fixed two warnings, and I left one as I'm not going to screw the code readability for this https://sonarcloud.io/project/issues?issues=AZF7JkDXShDQxsHwFJxz&open=AZF7JkDXShDQxsHwFJxz&pullRequest=1477&id=akuker-PISCSI :)

Right now I really don't have time to add unit test for all the new code, so the code coverage will remain as it is.

@rdmark
Copy link
Member

rdmark commented Aug 23, 2024

I could not managed link the tape files extensions (.tar, .tap) to the SCST (scisi streamer)

The extension mapping is handled in https://github.com/PiSCSI/piscsi/blob/develop/cpp/devices/device_factory.h

The Python client is just a dumb front end in this aspect.

I could not managed to add a list of common tape sizes for uses to create new images

I see that you added the one Tandberg device profile with 250MB image size to device_properties.json. This is the one mechanism we have for providing common image sizes presently. Do you have another mechanism in mind?

After I eject the current streamer file, I can not pick a new one, the drop down combobox is empty.

You need to add a new if block here for the SCST device type:

{% if device.device_type == "SCCD" %}

@bog-dan-ro
Copy link
Author

I could not managed link the tape files extensions (.tar, .tap) to the SCST (scisi streamer)

The extension mapping is handled in https://github.com/PiSCSI/piscsi/blob/develop/cpp/devices/device_factory.h

The Python client is just a dumb front end in this aspect.

Done.

I could not managed to add a list of common tape sizes for uses to create new images

I see that you added the one Tandberg device profile with 250MB image size to device_properties.json. This is the one mechanism we have for providing common image sizes presently. Do you have another mechanism in mind?

Nope, it's actually fine as it's very easy for users to add new images without recompiling the cpp bits.

After I eject the current streamer file, I can not pick a new one, the drop down combobox is empty.

You need to add a new if block here for the SCST device type:

{% if device.device_type == "SCCD" %}

I added an entry also here

{% if type == "SCMO" %}

Sadly I did all the web changes blindly as I could not test it because docker compose from docker folder didn't worked for me.

Also for the next two week I'll not have a computer near by, so either you'll wait for me to comeback, or please feel free to fix the patch for me.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
10.8% Coverage on New Code (required ≥ 60%)
13.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@rdmark
Copy link
Member

rdmark commented Aug 24, 2024

Did you follow the docker instructions in https://github.com/PiSCSI/piscsi/tree/develop/docker ? What issues did you run into? If there is something that’s broken then I’d like to try fixing it!

@bog-dan-ro
Copy link
Author

Did you follow the docker instructions in https://github.com/PiSCSI/piscsi/tree/develop/docker ? What issues did you run into? If there is something that’s broken then I’d like to try fixing it!

Yup, I think I did that. I can see the main page but I can not login with user: pi pass:

@rdmark
Copy link
Member

rdmark commented Aug 24, 2024

I think the most sure way to get it to work is to define the BACKEND_PASSWORD environment variable with the password of your choice.

@bog-dan-ro
Copy link
Author

It didn't work... I'll check it again once I'm back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tape support
3 participants