Fix daemon and python 3.7 issues #150

gonzoleeman · 2019-10-07T22:49:06Z

Fixed targetcli/targetclid interaction, both with and without systemd.

Tested on 5.3 kernel, and with python 3.7, 4.12 kernel with python 3.6.5.

The indentation was wrong, causing the daemon to only process the very last line of outout.

We can no longer have python automatically convert binary to/from string data, so be explicit about it. Also, the TargetCLI __del__ method was testing for a non-existant attribute.

There's no need to reply when we have a zero-byte message. Before being fixed, this caused a hang when shutting down.

Save the error exit for unknown exits. This makes systemd happier, since it stops us with a signal.

This allows the main loop to get an exception. Before this, stopping the daemon required sending a KILL signal or sending one last command, so that the loop could check the "received a signal" flag.

Regular socket communications are fallen back to when no systemd socket is present.

The 'exit' command returns nothing, but we must return at least that blank response to make targetcli happy.

Before, we would get a "Bad File Descriptor" message when a signal causes closure of the socket while waiting on a socket message.

pkalever

Have tested the series, the unix sock activation works as expected now.

Thank you very much @gonzoleeman I have now understand which piece is missing :-)

And thanks for correcting all the indentations. What I like most are those short and crisp cuts of various patches and good commit messages :-)

Adding few minor review comments.

pkalever · 2019-10-09T11:00:14Z

systemd/targetclid.service

-After=network.target targetclid.socket
-Requires=targetclid.socket
+Documentation=man:targetcli(8)
+After=network.target targetcli.service


may be you mean targetclid.socket ?

pkalever · 2019-10-09T11:01:44Z

daemon/targetclid

@@ -92,8 +92,7 @@ class TargetCLI:
        '''
        destructor
        '''
-        if not self.pfd.closed:
-            self.pfd.close()
+        self.pdf.close()


should this be s/pdf/pfd/ ?

and may be, I was thinking something like:

if self.pfd: self.pdf.close()

pkalever · 2019-10-09T11:03:33Z

daemon/targetclid

@@ -92,7 +92,7 @@ class TargetCLI:
        '''
        destructor
        '''
-        self.pdf.close()
+        self.pfd.close()


I see you fixed the typo here, instead may be you want to merge this patch to patch with title "Handle Python 3.7 stricter binary vs. string rules."

pkalever · 2019-10-09T12:04:33Z

daemon/targetclid

@@ -225,6 +228,7 @@ def main():
        to.display(to.render(err.strerror, 'red'))
        sys.exit(1)

+    to.sock = sock


This is a good catch :-)

Can we assign it as soon as it is created ? may be at line 216 ?

pkalever · 2019-10-09T13:26:35Z

daemon/targetclid

@@ -164,9 +164,9 @@ class TargetCLI:

                with open('/tmp/data.txt', 'r') as f:
                    output = f.read()
+                    var = struct.pack('i', len(output))
+                    connection.sendall(var)    # length of string


May be club this patch with patch with subject "Only return response to targetcli when bytes present"

gonzoleeman · 2019-10-10T01:27:22Z

On Oct 9, 2019, at 6:30 AM, Prasanna Kumar Kalever ***@***.***> wrote: @pkalever commented on this pull request. Have tested the series, the unix sock activation works as expected now. Thank you very much @gonzoleeman <https://github.com/gonzoleeman> I have now understand which piece is missing :-)

It wasn’t very much. Systemd already does half the socket work with socket activation.

And thanks for correcting all the indentations. What I like most are those short and crisp cuts of various patches and good commit messages :-)

There was only one place that was incorrect, which only effected passing in multiple commands, I believe.

Adding few minor review comments. In systemd/targetclid.service <#150 (comment)>: > @@ -1,7 +1,7 @@ [Unit] Description=Targetcli daemon -After=network.target targetclid.socket -Requires=targetclid.socket +Documentation=man:targetcli(8) +After=network.target targetcli.service may be you mean targetclid.socket ?

No, I do not. It does not need to be told to start after targetclid.socket. In SUSE we have targetcli.service, which manages the targetcli configuration persistence. I will be proposing that same service file for targetcli-fb soon, but for you it doesn’t exist yet. Which means this “After” doesn’t hurt anything. I can remove it, for now, if you wish.

In daemon/targetclid <#150 (comment)>: > @@ -92,8 +92,7 @@ class TargetCLI: ''' destructor ''' - if not self.pfd.closed: - self.pfd.close() + self.pdf.close() should this be s/pdf/pfd/ ?

This is fixed in further commits.

and may be, I was thinking something like: if self.pfd: self.pdf.close()

I _believe_ calling close() on an FD that is not open is not an exception, but I may be wrong.

In daemon/targetclid <#150 (comment)>: > @@ -92,7 +92,7 @@ class TargetCLI: ''' destructor ''' - self.pdf.close() + self.pfd.close() I see you fixed the typo here, instead may be you want to merge this patch to patch with title "Handle Python 3.7 stricter binary vs. string rules."

LOL. I would rather leave the fix in place, but I understand if you wish each commit to stand alone (though the desctructor was already broken, before). If you wish, I will redo my fixes, eliminating the need for the “typo fix” commit.

In daemon/targetclid <#150 (comment)>: > @@ -225,6 +228,7 @@ def main(): to.display(to.render(err.strerror, 'red')) sys.exit(1) + to.sock = sock This is a good catch :-) Can we assign it as soon as it is created ? may be at line 216 ?

Yes. But note that I created the “to.sock” attribute, so that it can be closed when we receive a signal, thus ending the listening loop.

In daemon/targetclid <#150 (comment)>: > @@ -164,9 +164,9 @@ class TargetCLI: with open('/tmp/data.txt', 'r') as f: output = f.read() + var = struct.pack('i', len(output)) + connection.sendall(var) # length of string May be club this patch with patch with subject "Only return response to targetcli when bytes present"

I assume you mean combine, since I don’t golf.

…

-- The Lee-Man Rides Again "...And maddest of all, to see life as it is and not as it should be!" — Cervantes

pkalever · 2019-10-10T07:24:00Z

No, I do not. It does not need to be told to start after targetclid.socket.

In SUSE we have targetcli.service, which manages the targetcli configuration persistence. I will be proposing that same service file for targetcli-fb soon, but for you it doesn’t exist yet. Which means this “After” doesn’t hurt anything.

I can remove it, for now, if you wish.

Ah! okay. Yes, for now IMO it will be confusing for users/readers when they cat the targetclid.service file and see something that is not part of installation.

May be for now you can have a wrapper service in suse project which should import targetclid.service and play your stuff there ?

I believe calling close() on an FD that is not open is not an exception, but I may be wrong.

Yeah, my intention was to just follow any uniform style within this file at-least, example we used,

if self.sock:
    self.sock.close()

I'm okay with both the ways.

Yes. But note that I created the “to.sock” attribute, so that it can be closed when we receive a signal, thus ending the listening loop.

Not sure, if I was able to put my intention correctly before, what I meant was, lets assign sock to to.sock attribute as soon as sock is created (at 216 ?), so that if the signal is received before current assignement (before entering the listening loop) the socket will be closed in the sig handler.

Rest of my comments are not mandatory, but if you are re-spinning the series, feel free to consider or drop them as per your choice :-)

Thanks!

gonzoleeman · 2019-10-10T21:05:49Z

I am submitting an updated pull request, so I'm closing this one.

gonzoleeman added 12 commits October 7, 2019 11:54

Fix indention for targetclid when processing output.

0c4bca2

The indentation was wrong, causing the daemon to only process the very last line of outout.

Handle Python 3.7 stricter binary vs. string rules.

d69e26c

We can no longer have python automatically convert binary to/from string data, so be explicit about it. Also, the TargetCLI __del__ method was testing for a non-existant attribute.

Tweak systemd socket-activation settings for daemon

95bd85e

Fix typo for TargetCLI destructor: pdf -> pfd

037d06b

Handle OSError correctly: use strerror to get string

0bcf6ef

Removed useless semicolons, as they're ignored

225edf5

Only return response to targetcli when bytes present

e93522f

There's no need to reply when we have a zero-byte message. Before being fixed, this caused a hang when shutting down.

Exit with success when getting a signal.

96a3430

Save the error exit for unknown exits. This makes systemd happier, since it stops us with a signal.

Close socket when receiving a signal to interrupt connection.

38543f7

This allows the main loop to get an exception. Before this, stopping the daemon required sending a KILL signal or sending one last command, so that the loop could check the "received a signal" flag.

Handle systemd socket activation, when present.

c616194

Regular socket communications are fallen back to when no systemd socket is present.

Fix issue with passing 'exit' as a command.

9d20a39

The 'exit' command returns nothing, but we must return at least that blank response to make targetcli happy.

Do not print err msg when signal closes socket.

caeeca3

Before, we would get a "Bad File Descriptor" message when a signal causes closure of the socket while waiting on a socket message.

gonzoleeman mentioned this pull request Oct 7, 2019

Problems using new targetclid daemon #149

Open

pkalever reviewed Oct 9, 2019

View reviewed changes

gonzoleeman closed this Oct 10, 2019

gonzoleeman deleted the fix-daemon-and-python-3.7-issues branch February 18, 2020 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix daemon and python 3.7 issues #150

Fix daemon and python 3.7 issues #150

gonzoleeman commented Oct 7, 2019

pkalever left a comment

pkalever Oct 9, 2019

pkalever Oct 9, 2019

pkalever Oct 9, 2019

pkalever Oct 9, 2019

pkalever Oct 9, 2019

gonzoleeman commented Oct 10, 2019 via email

pkalever commented Oct 10, 2019

gonzoleeman commented Oct 10, 2019

Fix daemon and python 3.7 issues #150

Fix daemon and python 3.7 issues #150

Conversation

gonzoleeman commented Oct 7, 2019

pkalever left a comment

Choose a reason for hiding this comment

pkalever Oct 9, 2019

Choose a reason for hiding this comment

pkalever Oct 9, 2019

Choose a reason for hiding this comment

pkalever Oct 9, 2019

Choose a reason for hiding this comment

pkalever Oct 9, 2019

Choose a reason for hiding this comment

pkalever Oct 9, 2019

Choose a reason for hiding this comment

gonzoleeman commented Oct 10, 2019 via email

pkalever commented Oct 10, 2019

gonzoleeman commented Oct 10, 2019