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

try more default socket paths #795

Merged
merged 10 commits into from
May 23, 2020
Merged

try more default socket paths #795

merged 10 commits into from
May 23, 2020

Conversation

gfrlv
Copy link
Member

@gfrlv gfrlv commented Nov 4, 2019

Description

Issue #794
If the socket path is not specified, try to find a file by the name mask mysql*.socket in different locations.

I wonder if we should move DEFAULT_SOCKET_DIRS from code to .myclirc, so it can be configured for different distributions.

WIP: test and collect more data on where the default socket may be placed in different OS.

Checklist

  • I've added this contribution to the changelog.md.
  • I've added my name to the AUTHORS file (or it's already there).

@amjith
Copy link
Member

amjith commented Dec 9, 2019

Is this ready for a merge?

@gfrlv
Copy link
Member Author

gfrlv commented Dec 9, 2019

Not quite: it works on the distributions from the table, but I still have not gotten to inspecting other distributions, and, more importantly, other DBs like MariaDB (they may name sockets differently).

By the way, does anyone have access to a MacOS system? What is the MySQL socket path there?

@@ -3,11 +3,13 @@
from mycli.encodingutils import text_type
import os

DEFAULT_SOCKET_DIRS = ('/var/run/', '/var/lib/', '/tmp')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/tmp might not be such a good idea; on a multi-user system anyone might create a socket in this directory to intercept passwords...

@meeuw
Copy link
Contributor

meeuw commented Dec 14, 2019

I like this change because it will help a common problem experienced by our users.

If we support multiple socket locations we should inform the user which location was chosen, especially when there are actually multiple socket locations available.

@j-bennet
Copy link
Contributor

@pasenor I'm on macOs and it is /tmp/mysql.sock:

$ mysql_config --socket
/tmp/mysql.sock

@gfrlv
Copy link
Member Author

gfrlv commented Dec 25, 2019

OK then, let's add /tmp only for MacOS, to do justice to @meeuw 's security concern. After all, a mac is typically a personal system.

@meeuw
Copy link
Contributor

meeuw commented Dec 25, 2019

OK then, let's add /tmp only for MacOS, to do justice to @meeuw 's security concern. After all, a mac is typically a personal system.

That would be fine for me, another posibility would be to check the owner of the socket (root? mysql?)

@gfrlv gfrlv changed the title [WIP] try more default socket paths try more default socket paths Mar 3, 2020
@amjith
Copy link
Member

amjith commented Apr 27, 2020

I'm seeing an error when I launch mycli with no arguments or options.

Stacktrace from the log file:

'Traceback (most recent call last):\n  File
"/home/amjith/Dropbox/code/python/mycli/mycli/main.py", line 450, in connect\n
_connect()\n  File "/home/amjith/Dropbox/code/python/mycli/mycli/main.py", line
419, in _connect\n    ssh_password, ssh_key_filename\n  File
"/home/amjith/Dropbox/code/python/mycli/mycli/sqlexecute.py", line 62, in
__init__\n    self.connect()\n  File
"/home/amjith/Dropbox/code/python/mycli/mycli/sqlexecute.py", line 118, in
connect\n    defer_connect=defer_connect\n  File
"/home/amjith/.virtualenvs/mycli/lib/python3.7/site-packages/pymysql/__init__.py",
line 94, in Connect\n    return Connection(*args, **kwargs)\n  File
"/home/amjith/.virtualenvs/mycli/lib/python3.7/site-packages/pymysql/connections.py",
line 325, in __init__\n    self.connect()\n  File
"/home/amjith/.virtualenvs/mycli/lib/python3.7/site-packages/pymysql/connections.py",
line 598, in connect\n    self._get_server_information()\n  File
"/home/amjith/.virtualenvs/mycli/lib/python3.7/site-packages/pymysql/connections.py",
line 985, in _get_server_information\n    self.server_thread_id =
struct.unpack(\'<I\', data[i:i+4])\nstruct.error: unpack requires a buffer of 4
bytes\n'

@gfrlv
Copy link
Member Author

gfrlv commented Apr 28, 2020

My guess would be that we found a socket (a wrong one), connected to it, but now it is sending garbage. Is it likely to happen in your setup (i.e. do you have multiple socket candidates)? I'll try to reproduce the situation and push a workaround. Also, we only echo the socket location after a successful connect, probably better to do it earlier.

@gfrlv gfrlv force-pushed the default-socket-location branch from 016badc to d70ce56 Compare May 10, 2020 19:35
@gfrlv
Copy link
Member Author

gfrlv commented May 10, 2020

i looked into it again and found another problem: while reading the .cnf files we did follow the !includedirs directives. If the socket is defined in one of those included files, it was not found and mycli had to guess.

With this fixed, the guessing function should only be called as a last resort in case our user is not allowed to read configs.

@amjith can you check if your error still happens? I have not been able to reproduce it, but If the cause was mycli finding the wrong socket it may be fixed now.

@gfrlv gfrlv force-pushed the default-socket-location branch from d70ce56 to a3cf82a Compare May 10, 2020 19:50
@codecov-io
Copy link

codecov-io commented May 10, 2020

Codecov Report

Merging #795 into master will decrease coverage by 0.28%.
The diff coverage is 67.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #795      +/-   ##
==========================================
- Coverage   79.70%   79.41%   -0.29%     
==========================================
  Files          23       23              
  Lines        1754     1793      +39     
==========================================
+ Hits         1398     1424      +26     
- Misses        356      369      +13     
Impacted Files Coverage Δ
mycli/sqlexecute.py 86.01% <ø> (ø)
mycli/packages/filepaths.py 61.36% <31.25%> (-17.21%) ⬇️
mycli/config.py 88.88% <91.66%> (+0.42%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84dcba2...a3cf82a. Read the comment docs.

@gfrlv gfrlv mentioned this pull request May 23, 2020
@amjith
Copy link
Member

amjith commented May 23, 2020

Works well. Thank you!

🏎️

@amjith amjith merged commit 7a059a6 into master May 23, 2020
@gfrlv gfrlv deleted the default-socket-location branch May 24, 2020 14:49
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.

5 participants