diff mbox series

[02/10] python/aqmp: rename 'accept()' to 'start_server_and_accept()'

Message ID 20220225205948.3693480-3-jsnow@redhat.com
State New
Headers show
Series Python: Fix qmp race condition on accept() | expand

Commit Message

John Snow Feb. 25, 2022, 8:59 p.m. UTC 
Previously, I had a method named "accept()" that under-the-hood calls
bind(2), listen(2) *and* accept(2). I meant this as a simplification and
counterpart to the one-shot "connect()" method.

This is confusing to readers who expect accept() to mean *just*
accept(2). Since I need to split apart the "accept()" method into
multiple methods anyway (one of which strongly resembling accept(2)), it
feels pertinent to rename this method *now*.

Rename this all-in-one method "start_server_and_accept()" instead.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/aqmp/legacy.py   |  2 +-
 python/qemu/aqmp/protocol.py |  6 ++++--
 python/tests/protocol.py     | 24 ++++++++++++------------
 3 files changed, 17 insertions(+), 15 deletions(-)

Comments

Daniel P. Berrangé March 4, 2022, 5:48 p.m. UTC | #1 
On Fri, Feb 25, 2022 at 03:59:40PM -0500, John Snow wrote:
> Previously, I had a method named "accept()" that under-the-hood calls
> bind(2), listen(2) *and* accept(2). I meant this as a simplification and
> counterpart to the one-shot "connect()" method.
> 
> This is confusing to readers who expect accept() to mean *just*
> accept(2). Since I need to split apart the "accept()" method into
> multiple methods anyway (one of which strongly resembling accept(2)), it
> feels pertinent to rename this method *now*.
> 
> Rename this all-in-one method "start_server_and_accept()" instead.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/aqmp/legacy.py   |  2 +-
>  python/qemu/aqmp/protocol.py |  6 ++++--
>  python/tests/protocol.py     | 24 ++++++++++++------------
>  3 files changed, 17 insertions(+), 15 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
diff mbox series

Patch

diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py
index 6baa5f3409..dca1e76ed4 100644
--- a/python/qemu/aqmp/legacy.py
+++ b/python/qemu/aqmp/legacy.py
@@ -91,7 +91,7 @@  def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage:
         self._aqmp.negotiate = True
 
         self._sync(
-            self._aqmp.accept(self._address),
+            self._aqmp.start_server_and_accept(self._address),
             timeout
         )
 
diff --git a/python/qemu/aqmp/protocol.py b/python/qemu/aqmp/protocol.py
index 009883f64d..73719257e0 100644
--- a/python/qemu/aqmp/protocol.py
+++ b/python/qemu/aqmp/protocol.py
@@ -265,8 +265,10 @@  async def runstate_changed(self) -> Runstate:
 
     @upper_half
     @require(Runstate.IDLE)
-    async def accept(self, address: SocketAddrT,
-                     ssl: Optional[SSLContext] = None) -> None:
+    async def start_server_and_accept(
+            self, address: SocketAddrT,
+            ssl: Optional[SSLContext] = None
+    ) -> None:
         """
         Accept a connection and begin processing message queues.
 
diff --git a/python/tests/protocol.py b/python/tests/protocol.py
index 5cd7938be3..354d6559b9 100644
--- a/python/tests/protocol.py
+++ b/python/tests/protocol.py
@@ -413,14 +413,14 @@  async def _bad_connection(self, family: str):
         assert family in ('INET', 'UNIX')
 
         if family == 'INET':
-            await self.proto.accept(('example.com', 1))
+            await self.proto.start_server_and_accept(('example.com', 1))
         elif family == 'UNIX':
-            await self.proto.accept('/dev/null')
+            await self.proto.start_server_and_accept('/dev/null')
 
     async def _hanging_connection(self):
         with TemporaryDirectory(suffix='.aqmp') as tmpdir:
             sock = os.path.join(tmpdir, type(self.proto).__name__ + ".sock")
-            await self.proto.accept(sock)
+            await self.proto.start_server_and_accept(sock)
 
 
 class FakeSession(TestBase):
@@ -449,13 +449,13 @@  async def testFakeConnect(self):
     @TestBase.async_test
     async def testFakeAccept(self):
         """Test the full state lifecycle (via accept) with a no-op session."""
-        await self.proto.accept('/not/a/real/path')
+        await self.proto.start_server_and_accept('/not/a/real/path')
         self.assertEqual(self.proto.runstate, Runstate.RUNNING)
 
     @TestBase.async_test
     async def testFakeRecv(self):
         """Test receiving a fake/null message."""
-        await self.proto.accept('/not/a/real/path')
+        await self.proto.start_server_and_accept('/not/a/real/path')
 
         logname = self.proto.logger.name
         with self.assertLogs(logname, level='DEBUG') as context:
@@ -471,7 +471,7 @@  async def testFakeRecv(self):
     @TestBase.async_test
     async def testFakeSend(self):
         """Test sending a fake/null message."""
-        await self.proto.accept('/not/a/real/path')
+        await self.proto.start_server_and_accept('/not/a/real/path')
 
         logname = self.proto.logger.name
         with self.assertLogs(logname, level='DEBUG') as context:
@@ -493,7 +493,7 @@  async def _prod_session_api(
     ):
         with self.assertRaises(StateError) as context:
             if accept:
-                await self.proto.accept('/not/a/real/path')
+                await self.proto.start_server_and_accept('/not/a/real/path')
             else:
                 await self.proto.connect('/not/a/real/path')
 
@@ -504,7 +504,7 @@  async def _prod_session_api(
     @TestBase.async_test
     async def testAcceptRequireRunning(self):
         """Test that accept() cannot be called when Runstate=RUNNING"""
-        await self.proto.accept('/not/a/real/path')
+        await self.proto.start_server_and_accept('/not/a/real/path')
 
         await self._prod_session_api(
             Runstate.RUNNING,
@@ -515,7 +515,7 @@  async def testAcceptRequireRunning(self):
     @TestBase.async_test
     async def testConnectRequireRunning(self):
         """Test that connect() cannot be called when Runstate=RUNNING"""
-        await self.proto.accept('/not/a/real/path')
+        await self.proto.start_server_and_accept('/not/a/real/path')
 
         await self._prod_session_api(
             Runstate.RUNNING,
@@ -526,7 +526,7 @@  async def testConnectRequireRunning(self):
     @TestBase.async_test
     async def testAcceptRequireDisconnecting(self):
         """Test that accept() cannot be called when Runstate=DISCONNECTING"""
-        await self.proto.accept('/not/a/real/path')
+        await self.proto.start_server_and_accept('/not/a/real/path')
 
         # Cheat: force a disconnect.
         await self.proto.simulate_disconnect()
@@ -541,7 +541,7 @@  async def testAcceptRequireDisconnecting(self):
     @TestBase.async_test
     async def testConnectRequireDisconnecting(self):
         """Test that connect() cannot be called when Runstate=DISCONNECTING"""
-        await self.proto.accept('/not/a/real/path')
+        await self.proto.start_server_and_accept('/not/a/real/path')
 
         # Cheat: force a disconnect.
         await self.proto.simulate_disconnect()
@@ -576,7 +576,7 @@  async def _asyncTearDown(self):
     async def testSmoke(self):
         with TemporaryDirectory(suffix='.aqmp') as tmpdir:
             sock = os.path.join(tmpdir, type(self.proto).__name__ + ".sock")
-            server_task = create_task(self.server.accept(sock))
+            server_task = create_task(self.server.start_server_and_accept(sock))
 
             # give the server a chance to start listening [...]
             await asyncio.sleep(0)