Add method for creating volume label #31

andrzejressel · 2020-06-13T23:18:20Z

This PR adds method createLabel that will create special file for volume label that will be detected by blkid after latest changes.

I've tested it with my fork of Xen Orchestra: https://github.com/jereksel/xen-orchestra/tree/label and Ubuntu 20.04

Fixes #30

natevw · 2020-06-30T20:24:04Z

Hi sorry for the delay on this. I think this is a good start and a reasonable approach overall, but have lots of specific feedback I'll add on the PR itself soon. (About to hop on yet another meeting call…)

natevw

Thanks much for this PR and including tests. Provided lots of feedback on some of the details but hopefully those are easy fixes and the core technique relying on fs._updateEntry looks good to me!

natevw · 2020-06-30T20:28:37Z

.gitignore

@@ -1,3 +1,5 @@
 .DS_Store
 .vscode/
 node_modules/
+package-lock.json
+label.img


Is it possible to create this image dynamically as a temp file instead of checking it in?

Oh silly me… was in a hurry and not realizing this was in the .gitignore and the whole point is that this is a temp file that is not getting tracked. Consider changing to the explicit path expected though? tests/label.img

natevw · 2020-06-30T21:05:55Z

index.js

@@ -557,6 +557,22 @@ exports.createFileSystem = function (volume, opts, cb) {
        else _.delayedCall(cb, S.err.NOSYS());
    }, (_n_ === '_nested_')); };

+    fs.createLabel = function (name, cb) {
+        fs.writeFile(name, "", () => {
+            fs.open(name, "w", (e,_fd) => {


Would it be possible to use simply one fs.open(…, "a") call here instead nesting of fs.write/fs.open?

natevw · 2020-06-30T21:10:38Z

package.json

+    "label-testcase": "nodejs tests/label.js"
+  },
+  "devDependencies": {
+    "promise-toolbox": "0.15.1"


See note below on usage.

natevw · 2020-06-30T21:15:20Z

tests/label.js

+  fatfs.createFileSystem(fatfsBuffer(buffer), {allowLowercaseNames: true})
+)
+
+createLabel("cidata").then(() => {


I don't think it's worth bringing in a new dependency just for one call. Please match the "old school" callback style used by the rest of the project here.

natevw · 2020-06-30T21:18:31Z

tests/fatfs-buffer.js

@@ -0,0 +1,79 @@
+//From https://github.com/vatesfr/xen-orchestra/blob/9f0b22d3e9c4672d96cb3f63b1e0fd42609e2406/packages/xo-server/src/fatfs-buffer.js


Is this code restricted by the AGPL3 license then, or is its author willing to contribute it under the MIT/Apache terms of this library?

I think I will replace it with example driver from readme and mkfs (https://superuser.com/a/668534)

natevw · 2020-06-30T21:33:40Z

tests/label.js

+var fatfsBufferInit = require('./fatfs-buffer').init
+var fs = require('fs')
+
+const buffer = fatfsBufferInit({ label: 'cidata     ' })


My understanding was that the buffer code from the xen-orchestra was a workaround before the createLabel method you're adding. So is it possible that passing in the same "cidata" label here will mask failures of the new method itself? Consider passing a recognizable "wrong" label here instead.

marlluslustosa · 2020-07-20T14:19:57Z

I tested this branch and recompiled XO locally and it didn't work.
Is that functional?

andrzejressel · 2020-07-21T19:31:18Z

I tested this branch and recompiled XO locally and it didn't work.
Is that functional?

Yeah, it should work fine. Could you please provide output of blkid /dev/cdrom and dosfslabel /dev/cdrom. I think you could also try to debug cloud-init. I can't remember how I've done it, but you can invoke only source search.

andrzejressel · 2020-07-21T19:35:43Z

Sorry that it took so long. I've fixed PR all comments

marlluslustosa · 2020-07-21T20:28:32Z

Cool. I am currently working to fix this on cloudinit. I have now sent patch patches to cloudinit both to see LABEL and LABEL_FATBOOT in their own scripts.

I hope to see this resolved by both parties as soon as possible.
You can check my update here: https://bugs.launchpad.net/cloud-init/+bug/1841466/comments/5
thanks!

natevw

Looking good, just one hopefully final bit of feedback!

natevw · 2020-07-21T21:01:15Z

index.js

+    fs.createLabel = function (name, cb) {
+        fs.open(name, "a", (e,_fd) => {
+            if (e) {
+                throw '_open_error_'


should be return cb(e);

Is return required? I don't think callback has to return anything.

natevw · 2023-05-30T22:07:53Z

D'oh! Sorry to let this one linger, but thanks for cleaning it up if it's no longer needed.

andrzejressel added 7 commits June 13, 2020 22:28

Support for label

f0a45bf

Support for label

a82c333

Remove lowercase stuff

af4a6e9

Remove lowercase stuff

3702394

Finish PR

a412f33

Merge branch 'label' of github.com:jereksel/fatfs into label

68eb1dd

Finish PR

8999e85

andrzejressel changed the title ~~Add method for create volume label~~ Add method for creating volume label Jun 14, 2020

natevw requested changes Jun 30, 2020

View reviewed changes

Fix PR comments

144d158

natevw requested changes Jul 21, 2020

View reviewed changes

Fix PR comments

d8b9cb7

andrzejressel closed this May 30, 2023

andrzejressel deleted the label branch May 30, 2023 21:51

MathieuRA mentioned this pull request Nov 22, 2024

fix(xo-server): support of cloudbase-init vatesfr/xen-orchestra#8154

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method for creating volume label #31

Add method for creating volume label #31

andrzejressel commented Jun 13, 2020 •

edited

Loading

natevw commented Jun 30, 2020

natevw left a comment

natevw Jun 30, 2020

natevw Jun 30, 2020

andrzejressel Jul 21, 2020

natevw Jun 30, 2020

andrzejressel Jul 21, 2020

natevw Jun 30, 2020

andrzejressel Jul 21, 2020

natevw Jun 30, 2020

andrzejressel Jul 21, 2020

natevw Jun 30, 2020

andrzejressel Jun 30, 2020

andrzejressel Jul 21, 2020

natevw Jun 30, 2020

andrzejressel Jul 21, 2020

marlluslustosa commented Jul 20, 2020

andrzejressel commented Jul 21, 2020

andrzejressel commented Jul 21, 2020

marlluslustosa commented Jul 21, 2020

natevw left a comment

natevw Jul 21, 2020

andrzejressel Jul 21, 2020

andrzejressel Jul 21, 2020

natevw commented May 30, 2023

		@@ -0,0 +1,79 @@
		//From https://github.com/vatesfr/xen-orchestra/blob/9f0b22d3e9c4672d96cb3f63b1e0fd42609e2406/packages/xo-server/src/fatfs-buffer.js

Add method for creating volume label #31

Add method for creating volume label #31

Conversation

andrzejressel commented Jun 13, 2020 • edited Loading

natevw commented Jun 30, 2020

natevw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marlluslustosa commented Jul 20, 2020

andrzejressel commented Jul 21, 2020

andrzejressel commented Jul 21, 2020

marlluslustosa commented Jul 21, 2020

natevw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natevw commented May 30, 2023

andrzejressel commented Jun 13, 2020 •

edited

Loading