Re: Please don't use bash when there are syscalls available
Thanks for the clarification, I misunderstood what the code was doing. I'm glad to hear this code won't be needed for much longer, but I think we should backport your explicit check so that non-english users can use Juju. On Tue, Sep 9, 2014 at 6:56 PM, Andrew Wilkins andrew.wilk...@canonical.com wrote: On Wed, Sep 10, 2014 at 4:45 AM, Nate Finch nate.fi...@canonical.com wrote: A user just complained that he can't bootstrap because Juju is parsing stderr text from flock, and his server isn't in English, so the error message isn't matching. https://github.com/juju/juju/blob/master/environs/sshstorage/storage.go#L254 Now, I think we all know that parsing error text is a bad idea, but I think I understand why it was done - it looks like flock the application only returns 1 on this failure, so it's not exactly a unique error code. However, flock the system call returns several different error codes, which are quite unique and easy to handle in a way that is not dependent on the language of the machine. It also happens to be already implemented in the syscalls package: http://golang.org/pkg/syscall/#Flock So let's fix this, and try not to call out to bash unless there's absolutely no other way. This is running on a remote system before there is any code deployed. I won't say there's *no other way*, but you can't invoke syscalls from out of thin air. Finally, that error message check has nothing to do with flock. It's checking the result of I/O redirection to base64. Agreed that parsing the message is dumb. We can fix this with an explicit file existence check in the command executed ((test -e $path || echo blah2)||base64$path) or so. In the not too distant future, we will have no need for sshstorage at all (except in past, supported releases). -Nate -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Please don't use bash when there are syscalls available
On Wed, Sep 10, 2014 at 6:57 PM, Nate Finch nate.fi...@canonical.com wrote: Thanks for the clarification, I misunderstood what the code was doing. I'm glad to hear this code won't be needed for much longer, but I think we should backport your explicit check so that non-english users can use Juju. I have filed https://bugs.launchpad.net/juju-core/+bug/1367695, assigned to 1.20.8. On Tue, Sep 9, 2014 at 6:56 PM, Andrew Wilkins andrew.wilk...@canonical.com wrote: On Wed, Sep 10, 2014 at 4:45 AM, Nate Finch nate.fi...@canonical.com wrote: A user just complained that he can't bootstrap because Juju is parsing stderr text from flock, and his server isn't in English, so the error message isn't matching. https://github.com/juju/juju/blob/master/environs/sshstorage/storage.go#L254 Now, I think we all know that parsing error text is a bad idea, but I think I understand why it was done - it looks like flock the application only returns 1 on this failure, so it's not exactly a unique error code. However, flock the system call returns several different error codes, which are quite unique and easy to handle in a way that is not dependent on the language of the machine. It also happens to be already implemented in the syscalls package: http://golang.org/pkg/syscall/#Flock So let's fix this, and try not to call out to bash unless there's absolutely no other way. This is running on a remote system before there is any code deployed. I won't say there's *no other way*, but you can't invoke syscalls from out of thin air. Finally, that error message check has nothing to do with flock. It's checking the result of I/O redirection to base64. Agreed that parsing the message is dumb. We can fix this with an explicit file existence check in the command executed ((test -e $path || echo blah2)||base64$path) or so. In the not too distant future, we will have no need for sshstorage at all (except in past, supported releases). -Nate -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Please don't use bash when there are syscalls available
A user just complained that he can't bootstrap because Juju is parsing stderr text from flock, and his server isn't in English, so the error message isn't matching. https://github.com/juju/juju/blob/master/environs/sshstorage/storage.go#L254 Now, I think we all know that parsing error text is a bad idea, but I think I understand why it was done - it looks like flock the application only returns 1 on this failure, so it's not exactly a unique error code. However, flock the system call returns several different error codes, which are quite unique and easy to handle in a way that is not dependent on the language of the machine. It also happens to be already implemented in the syscalls package: http://golang.org/pkg/syscall/#Flock So let's fix this, and try not to call out to bash unless there's absolutely no other way. -Nate -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Please don't use bash when there are syscalls available
Worth keeping in mind the usual gotcha: the API of syscall is different for different OSes. On Tue, Sep 9, 2014 at 5:45 PM, Nate Finch nate.fi...@canonical.com wrote: A user just complained that he can't bootstrap because Juju is parsing stderr text from flock, and his server isn't in English, so the error message isn't matching. https://github.com/juju/juju/blob/master/environs/sshstorage/storage.go#L254 Now, I think we all know that parsing error text is a bad idea, but I think I understand why it was done - it looks like flock the application only returns 1 on this failure, so it's not exactly a unique error code. However, flock the system call returns several different error codes, which are quite unique and easy to handle in a way that is not dependent on the language of the machine. It also happens to be already implemented in the syscalls package: http://golang.org/pkg/syscall/#Flock So let's fix this, and try not to call out to bash unless there's absolutely no other way. -Nate -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- gustavo @ http://niemeyer.net -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Please don't use bash when there are syscalls available
On Wed, Sep 10, 2014 at 4:45 AM, Nate Finch nate.fi...@canonical.com wrote: A user just complained that he can't bootstrap because Juju is parsing stderr text from flock, and his server isn't in English, so the error message isn't matching. https://github.com/juju/juju/blob/master/environs/sshstorage/storage.go#L254 Now, I think we all know that parsing error text is a bad idea, but I think I understand why it was done - it looks like flock the application only returns 1 on this failure, so it's not exactly a unique error code. However, flock the system call returns several different error codes, which are quite unique and easy to handle in a way that is not dependent on the language of the machine. It also happens to be already implemented in the syscalls package: http://golang.org/pkg/syscall/#Flock So let's fix this, and try not to call out to bash unless there's absolutely no other way. This is running on a remote system before there is any code deployed. I won't say there's *no other way*, but you can't invoke syscalls from out of thin air. Finally, that error message check has nothing to do with flock. It's checking the result of I/O redirection to base64. Agreed that parsing the message is dumb. We can fix this with an explicit file existence check in the command executed ((test -e $path || echo blah2)||base64$path) or so. In the not too distant future, we will have no need for sshstorage at all (except in past, supported releases). -Nate -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: Please don't use bash when there are syscalls available
If we do need to parse the error message, we could use the LANG=C trick. But yes, I'd rather avoid it if possible. John =:- On Wed, Sep 10, 2014 at 2:56 AM, Andrew Wilkins andrew.wilk...@canonical.com wrote: On Wed, Sep 10, 2014 at 4:45 AM, Nate Finch nate.fi...@canonical.com wrote: A user just complained that he can't bootstrap because Juju is parsing stderr text from flock, and his server isn't in English, so the error message isn't matching. https://github.com/juju/juju/blob/master/environs/sshstorage/storage.go#L254 Now, I think we all know that parsing error text is a bad idea, but I think I understand why it was done - it looks like flock the application only returns 1 on this failure, so it's not exactly a unique error code. However, flock the system call returns several different error codes, which are quite unique and easy to handle in a way that is not dependent on the language of the machine. It also happens to be already implemented in the syscalls package: http://golang.org/pkg/syscall/#Flock So let's fix this, and try not to call out to bash unless there's absolutely no other way. This is running on a remote system before there is any code deployed. I won't say there's *no other way*, but you can't invoke syscalls from out of thin air. Finally, that error message check has nothing to do with flock. It's checking the result of I/O redirection to base64. Agreed that parsing the message is dumb. We can fix this with an explicit file existence check in the command executed ((test -e $path || echo blah2)||base64$path) or so. In the not too distant future, we will have no need for sshstorage at all (except in past, supported releases). -Nate -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
