Skip to content

Updated with Set Host Name fuctionality -Tested with Arduino 1.6.13 #50

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

technofreakz
Copy link

Updated to add Set Host Name functionality .
Added Example Sketch for set Host Name.
For Arduino 1.6.13

technofreakz added 4 commits September 19, 2017 19:19
Updated  to add Set Host Name fuctionality .
Added Example Sketch for set Host Name.
Formating of output message in Sketch,
Added linefeed
@per1234
Copy link
Contributor

per1234 commented Sep 19, 2017

DhcpHostName.ino

Please use the Arduino IDE to do a Tools > Auto Format, which will fix some trailing whitespace and indentation issues.

Dhcp.cpp, Ethernet.cpp

Please conform to the formatting standard established throughout the rest of this file and library by using spaces instead of tabs.

@dab0bby
Copy link

dab0bby commented Mar 6, 2018

Hey guys, just wanna know whats the update on this and whether this can be expected to be merged.
Would be extremely useful for my use case.

@per1234
Copy link
Contributor

per1234 commented Mar 7, 2018

Similar to arduino/Arduino#5701

@mrmorrandir
Copy link

mrmorrandir commented Sep 17, 2018

Have to ask too: is this extremely useful feature coming soon?
This would be great in an industrial environment like mine - infrastructure admins would be really happy!

@shoop shoop mentioned this pull request Nov 21, 2018
@Misiu
Copy link

Misiu commented Jun 8, 2020

@per1234 can this be merged?

@per1234
Copy link
Contributor

per1234 commented Jun 9, 2020

@Misiu, since @technofreakz never made the changes I requested 2.5 years ago, I would say no. However, I'm not the one who can decide whether or not this will be merged.

@technofreakz
Copy link
Author

Hello All,
I had made changes as @per1234 requested,looks like code did not update in github.
I will verify this later evening and commit changes.

Thank you all for feedback's.

Good Day.

@Misiu
Copy link

Misiu commented Jun 9, 2020

@technofreakz I've checked your changes yesterday and a simple sketch worked perfectly.
@per1234 who can merge this when it is ready?

Copy link
Contributor

@Rotzbua Rotzbua left a comment

Choose a reason for hiding this comment

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

Just a quick look on the pr but the code has some more flaws in my opinion.

else
{
buffer[16] = hostName;
buffer[17] = strlen(HOST_NAME); // length of hostname + last 3 bytes of mac address
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong comment.

{
buffer[16] = hostName;
buffer[17] = strlen(HOST_NAME); // length of hostname + last 3 bytes of mac address
strcpy((char*)&(buffer[18]), HOST_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible buffer overflow.

@@ -9,6 +9,12 @@
#include "Arduino.h"
#include "utility/util.h"

char HOST_NAME[12] = "WIZNet";
Copy link
Contributor

Choose a reason for hiding this comment

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

UPPERCASE is for constants.

char HOST_NAME[12] = "WIZNet";

void DhcpClass::setHostName(char *dhcpHost) {
strcpy(HOST_NAME, dhcpHost);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing sanity check.

@@ -172,6 +172,9 @@ class DhcpClass {

int beginWithDHCP(uint8_t *, unsigned long timeout = 60000, unsigned long responseTimeout = 4000);
int checkLease();
//
void setHostName(char *);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing documentation: min max value; type of input: char array vs string.

@@ -27,6 +27,9 @@ class EthernetClass {
void begin(uint8_t *mac_address, IPAddress local_ip, IPAddress dns_server, IPAddress gateway, IPAddress subnet);
int maintain();

void hostName(char *dhcp_HostName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Misleading naming. Should begin with set.

char *EthernetClass::getHostName()
{
char *_getDhcpHostName = _dhcp ->getHostName();
return _getDhcpHostName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be simplified with previous line.

@PaulStoffregen
Copy link
Contributor

Please use File > Preferences to set Warnings to All.

setHostName() should take a const char * as its input? Without const, passing a string literal will generate compiler warnings.

The use of strcpy() also appears to be unsafe. It should check the string length and avoid overwriting beyond the end of the buffer.

@Misiu
Copy link

Misiu commented Jun 21, 2020

@technofreakz any updates on this?
Not sure about buffer overflow and sanity check, but the rest is very simple :)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


technofreakz seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

@mariodantas mariodantas left a comment

Choose a reason for hiding this comment

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

Working well

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.

10 participants