0

I'm learning win32 programming with C/C++. In the process of learning, my teacher wanted I write a simple program that it can shows the name of the computer which It runs on it and Then, if the name of the target computer was "USER", shows a warning in the output console. I written the following code, but It doesn't work.

myFunction Code : 

tchar * getComputerName() {
        bufCharCount = INFO_BUFFER_SIZE;
        if (!GetComputerName(infoBuf, &bufCharCount))
            printError(TEXT("GetComputerName"));
        return (TCHAR*)infoBuf;
    }

calling code : 

if (getComputerName() == (TCHAR*)"USER") {
            printf("Target OS Detected \n");
        }

how can i fix this issue?

Adonaim
  • 170
  • 1
  • 2
  • 11
  • 4
    Lots of problems. The memory allocation looks wonky. What is `infoBuf`? Why are you compelled to cast it? Why aren't you using std::string? Using `==` compares pointer addresses, not the contents of the strings. – David Heffernan Oct 22 '15 at 13:52
  • #define INFO_BUFFER_SIZE 32767 – Adonaim Oct 22 '15 at 13:56
  • TCHAR infoBuf[INFO_BUFFER_SIZE]; – Adonaim Oct 22 '15 at 13:57
  • And that's a global variable? Why? Why not use `std::string`? – David Heffernan Oct 22 '15 at 13:58
  • I'm a newbie CPP Programmer. I used std::string but i doesn't work too. I don't know how can fix this issue really. I just write a code which i thought it is correct. – Adonaim Oct 22 '15 at 13:59
  • Your code is broken in many ways. `==` is wrong. `strcmp` is how to compare the contents of C strings. But stop using C strings for the love of God. Use `std::string`. – David Heffernan Oct 22 '15 at 14:00
  • GetComputerName api doesn't accept string variable. That's problem. – Adonaim Oct 22 '15 at 14:05
  • No. So pass `&str[0]` then, after having allocated a sufficiently large enough buffer. You can find countless examples on the web of calling `GetComputerName` and transferring to `std::string`. – David Heffernan Oct 22 '15 at 14:07
  • It's a mistake to throw TCHAR into the mix. That was useful 15 years ago when we had to support Windows 98. Nowadays it's a terrible confusion. Use Unicode on Windows. Don't use TCHAR. – David Heffernan Oct 22 '15 at 15:08

3 Answers3

2

There are several issues with the code as posted. The most blatant one is the use of TCHARs. TCHAR was invented, before Win9x had Unicode support, in an attempt to keep code source code compatible between Win9x and Windows NT (the latter uses Unicode with UTF-16LE throughout). Today, there is no reason to use TCHARs at all. Simply use wchar_t and the Windows API calls with a W suffix.

The C-style casts (e.g. return (TCHAR*)infoBuf) are another error waiting to happen. If the code doesn't compile without a cast in this case, this means, you are using incompatible types (char vs. wchar_t).

Plus, there's a logical error: When using C-style strings (represented through pointers to a sequence of zero-terminated characters), you cannot use operator== to compare the string contents. It will compare the pointers instead. The solution to this is to either invoke an explicit string comparison (strcmp), or use a C++ string instead. The latter overloads operator== to perform a case-sensitive string compare.

A fixed version might look like this:

#include <windows.h>
#include <string>
#include <iostream>
#include <stdexcept>

std::wstring getComputerName() {
    wchar_t buffer[MAX_COMPUTERNAME_LENGTH + 1] = {0};
    DWORD cchBufferSize = sizeof(buffer) / sizeof(buffer[0]);
    if (!GetComputerNameW(buffer, &cchBufferSize))
        throw std::runtime_error("GetComputerName() failed.");
    return std::wstring(&buffer[0]);
}

int main() {
    const std::wstring compName = getComputerName();
    if ( compName == L"USER" ) {
        std::wcout << L"Print your message" << std::endl;
    }
}
IInspectable
  • 46,945
  • 8
  • 85
  • 181
  • Initialization of buffer is rather pointless and `bufferSize` is a poor name. That's a length rather than a size. – David Heffernan Oct 22 '15 at 17:12
  • @DavidHeffernan: How is proper initialization **ever** pointless? Provide succinct rationale, please. If you find `bufferSize` a poor name, file a Microsoft Connect documentation bug. It is derived from [GetComputerName](https://msdn.microsoft.com/en-us/library/windows/desktop/ms724295.aspx)'s formal parameter name. And if you want to get all nitpicky over this: A length doesn't include the `NUL` terminator. – IInspectable Oct 22 '15 at 17:22
  • I don't think it's fair that others aren't allowed to be nit picky. The call to the function guarantees to write to the buffer. If MS uses a poor name, you don't need to follow suit. Please be prepared to receive constructive criticism in the same way that you give it. – David Heffernan Oct 22 '15 at 17:35
  • @DavidHeffernan: There was nothing constructive about your comment. As I pointed out, this is **not** a length argument, since it accounts for the `NUL` terminator. And you still owe us a succinct rationale for not initializing a buffer. A *"code by coincidence"* excuse (as in your previous comment) won't do. – IInspectable Oct 22 '15 at 17:57
  • 1
    Your attitude is pretty appalling. You should mellow. You are very knowledgeable, but the rough edges need some serious work. – David Heffernan Oct 22 '15 at 18:03
  • @DavidHeffernan: You attempted to provide *"constructive criticism"*. When confronted, that it isn't, you failed to explain yourself. As it stands, what you called *"constructive"* simply isn't. Please, do address those *"rough edges"* in your reasoning. – IInspectable Oct 22 '15 at 18:12
  • I explained myself. It's fine that you don't agree. Your attitude is very strange. On the face of it, it looks like you are happy to criticise, but have trouble accepting criticism. There's no way that parameter can be considered a size. It's clearly a length. That is, the number of elements in the array. If you don't want to accept that, I don't mind. – David Heffernan Oct 22 '15 at 18:16
  • I should also say that I think your answer is, as your answers always are, excellent. You nail all the important points with clarity and precision. I offered two very minor points and you bit my head off. – David Heffernan Oct 22 '15 at 18:18
  • @DavidHeffernan: With all due respect, suggesting that not initializing a buffer were to improve code quality is not something you can disagree with. It is something you absolutely, positively **must** disagree with. Had I used the C++ equivalent (`std::vector`) I wouldn't even have a choice. It would be zero-initialized, no matter how hard I tried not to. As for the *length* vs. *size* argument: The argument resembles neither, so naming it either one is equally wrong. It is a size, but counted in characters, or a length, but including the `NUL` terminator. – IInspectable Oct 23 '15 at 07:47
  • `GetComputerNameW` initializes the buffer. Length and size have well established meanings that differ. Length is number of elements of an array, size is number of bytes. Length can also be used to mean the number of characters in a string, excluding the null terminator. But that's not relevant here. – David Heffernan Oct 23 '15 at 07:51
  • But perhaps count is a better name than length if you want to stick more closely to Windows naming conventions, such as they are. – David Heffernan Oct 23 '15 at 07:58
  • @DavidHeffernan: Nowhere in the [documentation](https://msdn.microsoft.com/en-us/library/windows/desktop/ms724295.aspx) does it say, that the in/out buffer is initialized. For all we know, all bytes trailing the `NUL` terminator could be uninitialized on return. As for the well established meanings of *length* and *size*: A well established standard [appears to disagree](http://en.cppreference.com/w/cpp/container/vector/size) with you. – IInspectable Oct 23 '15 at 08:04
  • But my main point is that it's fine to disagree and debate and argue over such points of detail. You do it all the time, with great clarity. But the caustic, contemptuous, dismissive tone is counter productive. It takes away from all the good you do. – David Heffernan Oct 23 '15 at 08:06
  • As far the C++ standard containers go, yes they use `size()` to mean number of elements. But we are interacting with a C API, Win32, which has different naming conventions. – David Heffernan Oct 23 '15 at 09:01
1

The following code works for me:

#include <windows.h>
// ...
std::string get_computer_name()
{
    const int buffer_size = MAX_COMPUTERNAME_LENGTH + 1;
    char buffer[buffer_size];
    DWORD lpnSize = buffer_size;
    if (GetComputerNameA(buffer, &lpnSize) == FALSE)
        throw std::runtime_error("Something went wrong.");
    return std::string{ buffer };
}
MSalters
  • 173,980
  • 10
  • 155
  • 350
Steve
  • 6,334
  • 4
  • 39
  • 67
  • How can i compare the return value of this function with "USER" ? I should use strcmp() ? – Adonaim Oct 22 '15 at 14:16
  • You should be able to simply do `if (get_computer_name() == "USER")`, because the `==` operator is overloaded for parameters of type `std::string` and `char*`. The reason this doesn't work in the code you posted is because you're comparing two `char*` types basically, which compares the value of the pointers, which is the memory address of the thing they're pointing to, not the value of the thing pointed to. – Steve Oct 22 '15 at 14:55
  • 1
    @the_milad Why would you use `strcmp`? That's for pointers to C strings. It seems to me that you don't really have much of a clue, but are continuing regardless. Instead of guessing wildly, you need to step back and consolidate your learning. It's pointless proceeding the way you are doing. Slow down! – David Heffernan Oct 22 '15 at 14:57
  • 1
    @DavidHeffernan You're right, thanks for pointing that out. I added a throw if it fails. – Steve Oct 22 '15 at 14:59
  • What if the [GetComputerName](https://msdn.microsoft.com/en-us/library/windows/desktop/ms724295.aspx) macro expands to `GetComputerNameW`, and now expects a `wchar_t*` argument? Unicode should really be considered the default, considering that Windows NT has been around for decades. – IInspectable Oct 22 '15 at 15:20
  • @IInspectable The link you posted (which is the reference I was originally using) says that it's a function, not a macro. Can it be replaced without warning? – Steve Oct 22 '15 at 15:23
  • It **is** a macro, like any other "function" in the Windows API, that takes a character string argument. The documentation you were using also states, that if you are using `GetComputerName`, the first argument is an `LPTSTR`, not an `LPSTR`. – IInspectable Oct 22 '15 at 15:25
  • @IInspectable: Fixed it. Using the UNICODE-dependent versions should be combined with `TCHAR`, and I agree that `TCHAR` is a bad idea. – MSalters Oct 22 '15 at 15:49
  • @MSalters Thanks :) Glad I posted an answer, you guys just improved my code too! – Steve Oct 22 '15 at 16:32
  • @MSalters: That's not a fix. While it does compile now, it doesn't match the question. The OP is clearly using `TCHAR`s, and the C-style cast implies, that `_UNICODE` is defined. Either a `TCHAR`-based, or a Unicode version should be used. – IInspectable Oct 22 '15 at 16:40
  • @IInspectable The asker has no idea what he's doing. He's using TCHAR through no choice of his own. He's just copied it from somewhere. Possible the hopeless msdn examples. – David Heffernan Oct 22 '15 at 17:09
-2

You can't compare two string pointers to compare the string.

DWORD dw_computer_name = MAX_COMPUTERNAME_LENGTH;
TCHAR computer_name[MAX_COMPUTERNAME_LENGTH+1];

if ( 0 != GetComputerName( computer_name, &dw_computer_name ) )
{
    printError( TEXT( "GetComputerName" ) );

    if ( 0 == _tcscmp( computer_name, _T("HOST") )
    {
        printf( "Target OS Detected \n" );
    }
}
Stan
  • 57
  • 5
  • It doesn't work yet. It's really makes me confused. – Adonaim Oct 22 '15 at 14:32
  • `windows.h` provides `MAX_COMPUTERNAME_LENGTH`, which should be used instead of the magic number `1024` for at least two reasons: 1) What if the computer name was greater than 1024 characters? 2) If `MAX_COMPUTERNAME_LENGTH` is substantially smaller than 1024, you're wasting a lot of stack space (I think this is the case, the maximum name is something like 16 characters or so if I remember right). – Steve Oct 22 '15 at 14:58
  • `(TCHAR*)"HOST"` should set the alarm bells going. Why did you feel compelled to cast? And when does that cast work? Answer, not always. What if `TCHAR` is `wchar_t`? Then the cast is bogus. Please stop casting. You used the `TEXT` macro once already, why not do so again? And why use TCHAR? It's 2015. Stick to Unicode text. – David Heffernan Oct 22 '15 at 15:07