Read only the http header

[flx5]

The VNC client won't connect if the version string has already been read.

[ddelnano]

Hi @flx5, thanks for taking the time to contribute. I've been taking some time off but I'll look at this later this week.

[flx5]

No worries.

I've just realized that this branch also includes a few other changes (sorry about that):

• It is now possible to specify the firmware (uefi or bios) in the config.
• Replace boot command with the code from qemu.

If those are problematic for you I guess I can try to create patch files for each of those.

Also a bit of background about why the change to reading the http request:

The unpatched version read the following (on XCP-ng 8.2):

Received response: HTTP/1.1 200 OK
==> xenserver-iso.test: Connection: keep-alive
==> xenserver-iso.test: Cache-Control: no-cache, no-store
==> xenserver-iso.test: 
==> xenserver-iso.test: RFB 003.008

The RFB 003.008 is supposed to be read by the vnc client though which is now waiting for the version string.

To fix this only the http header should be read from the connection before passing the connection on to the vnc client.

I haven't figured out how to avoid unbuffered reading though. Wrapping the connection in a bufio reader probably won't work because then the buffer contains the version string and the vnc client once again won't be capable of reading it directly from the tls connection.

[ddelnano]

I took a longer break than expected, but I'm getting back on things this week and will review this soon. Apologies for the long delay. Your contribution is very much appreciated!

[ddelnano]

Apologies once again but I've actually taken a look at your code this time :)

@flx5 can we please split the firmware changes outside of this PR? It will be easier to review that way.

As for the unbuffered reading, we should be able to accomplish that with Reader.Peek and Reader.ReadString. I've written this example below that compiles, but is untested. Please give that approach a try

diff --git a/builder/xenserver/common/step_type_boot_command.go b/builder/xenserver/common/step_type_boot_command.go
index 96ab373..c4dacb0 100644
--- a/builder/xenserver/common/step_type_boot_command.go
+++ b/builder/xenserver/common/step_type_boot_command.go
@@ -3,6 +3,7 @@ package common
 /* Heavily borrowed from builder/quemu/step_type_boot_command.go */

 import (
+       "bufio"
        "context"
        "crypto/tls"
        "fmt"
@@ -11,10 +12,10 @@ import (
        "net"
        "strings"

+       "github.com/hashicorp/packer-plugin-sdk/bootcommand"
        "github.com/hashicorp/packer-plugin-sdk/multistep"
        "github.com/hashicorp/packer-plugin-sdk/packer"
        "github.com/hashicorp/packer-plugin-sdk/template/interpolate"
-       "github.com/hashicorp/packer-plugin-sdk/bootcommand"
        "github.com/mitchellh/go-vnc"
 )

@@ -103,36 +104,40 @@ func (self *StepTypeBootCommand) Run(ctx context.Context, state multistep.StateB
                return multistep.ActionHalt
        }

-        // Look for \r\n\r\n sequence. Everything after the HTTP Header is for the vnc client.
-
-       builder := strings.Builder{}
-       buffer := make([]byte, 1)
-       sequenceProgress := 0
-
+       // Look for \r\n\r\n sequence. Everything after the HTTP Header is for the vnc client.
+       reader := bufio.NewReader(tlsConn)
+       var httpResp string
        for {
-               if _, err := io.ReadFull(tlsConn, buffer); err != nil {
+               httpResp, err = reader.ReadString('\r')
+               if err != nil {
                        err := fmt.Errorf("failed to start vnc session: %v", err)
                        state.Put("error", err)
                        ui.Error(err.Error())
                        return multistep.ActionHalt
                }

-               builder.WriteByte(buffer[0])
-
-               if buffer[0] == '\n' && sequenceProgress % 2 == 1 {
-                       sequenceProgress++
-               } else if buffer[0] == '\r' && sequenceProgress % 2 == 0 {
-                       sequenceProgress++
-               } else {
-                       sequenceProgress = 0
+               // Peek at the next three bytes to see if it contains the remaining \n\r\n
+               var b []byte
+               b, err = reader.Peek(3)
+               if err != nil {
+                       e := fmt.Errorf("failed to start vnc session: %v", err)
+                       state.Put("error", e)
+                       ui.Error(e.Error())
+                       return multistep.ActionHalt
                }

-               if sequenceProgress == 4 {
+               if b[0] == '\n' && b[1] == '\r' && b[2] == '\n' {
+                       if _, err = reader.Discard(3); err != nil {
+                               e := fmt.Errorf("failed to start vnc session: %v", err)
+                               state.Put("error", e)
+                               ui.Error(e.Error())
+                               return multistep.ActionHalt
+                       }
                        break
                }
        }
-
-       ui.Say(fmt.Sprintf("Received response: %s", builder.String()))
+
+       ui.Say(fmt.Sprintf("Received response: %s", httpResp))

[ddelnano]

Did you notice any bad behavior with the previous setting?

[ddelnano]

Can you remove the /* Heavily borrowed from builder/quemu/step_type_boot_command.go */ comment at the top of this file? That shouldn't be the case now that we are using the bootcommand package :)

[ddelnano]

Added a comment elsewhere with a git diff that shows how to avoid the unbuffered reading without consuming the buffer. Please give that a try.