[Spice-devel] [PATCH 4/4] vdi_port refactor: introduce old pci_vdi_port

[Arnon Gilboa]

Alon Levy wrote:
> This patch is a little dirty due to EOL convertion to windows format.
>   
isn't it dos eol in all files of this solution?
>  + add pci_vdi_port with PCIVDIPort taken from last commit before
>   changing to virtio-serial (a17ccbf323768c3cb977f0f062366ba7cf7f19db)
>  + move handle_error to VDIPort (identical in VIRTIOVDIPort and PCIVDIPort)
>  + make VDService create first a virtio, then init, the pci, then init,
>   stopping when the first init succeeds, and reporting to log which was
>   created.
> ---
>  common/vdlog.h                |    1 +
>  vdservice/pci_vdi_port.cpp    |  124 ++++++++++++++++++++++++++++++++++
>  vdservice/pci_vdi_port.h      |   54 +++++++++++++++
>  vdservice/vdi_port.cpp        |  150 ++++++++++++++++++++++------------------
>  vdservice/vdi_port.h          |    3 +
>  vdservice/vdservice.cpp       |   30 +++++++-
>  vdservice/vdservice.vcproj    |   16 +++++
>  vdservice/virtio_vdi_port.cpp |   15 ----
>   
vdservice/virtio_vdi_port.h is missing
>  8 files changed, 307 insertions(+),h>
>  #include <tchar.
>  86 deletions(-)
>  create mode 100644 vdservice/pci_vdi_port.cpp
>  create mode 100644 vdservice/pci_vdi_port.h
>
> diff --git a/common/vdlog.h b/common/vdlog.h
> index 419248a..bb2eb28 100644
> --- a/common/vdlog.h
> +++ b/common/vdlog.h
> @@ -18,6 +18,7 @@
>  #ifndef _H_VDLOG
>  #define _H_VDLOG
>  
> +#include <stdio.h>
>   
?
>  #include <tchar.h>
>  #include <crtdbg.h>
>  #include <windows.h>
> diff --git a/vdservice/pci_vdi_port.cpp b/vdservice/pci_vdi_port.cpp
> new file mode 100644
> index 0000000..3baefd9
> --- /dev/null
> +++ b/vdservice/pci_vdi_port.cpp
>   
<snip>
> +
> +void PCIVDIPort::handle_event(int event)
> +{
> +    // do nothing - the event merely serves to wake us up, then we call read/write
> +    // at VDService::execute start of while(_running) loop.
> +}
>   
so i guess it was virtualized for generality (e.g virtio class)

> diff --git a/vdservice/vdservice.cpp b/vdservice/vdservice.cpp
> index 7b0cadb..175587c 100644
> --- a/vdservice/vdservice.cpp
> +++ b/vdservice/vdservice.cpp
> @@ -23,6 +23,7 @@
>  #include <tlhelp32.h>
>  #include "vdcommon.h"
>  #include "virtio_vdi_port.h"
> +#include "pci_vdi_port.h"
>  
>  //#define DEBUG_VDSERVICE
>  
> @@ -401,6 +402,16 @@ VOID WINAPI VDService::main(DWORD argc, TCHAR* argv[])
>  #endif //DEBUG_VDSERVICE
>  }
>  
> +VDIPort *create_virtio_vdi_port()
> +{
> +    return new VirtioVDIPort();
> +}
> +
> +VDIPort *create_pci_vdi_port()
> +{
> +    return new PCIVDIPort();
> +}
> +
>  bool VDService::execute()
>  {
>      SECURITY_ATTRIBUTES sec_attr;
> @@ -434,12 +445,25 @@ bool VDService::execute()
>          CloseHandle(pipe);
>          return false;
>      }
> -    _vdi_port = new VirtioVDIPort();
> -    if (!_vdi_port->init()) {
> -        delete _vdi_port;
> +
> +    bool init = false;
> +    {
> +        VDIPort* (*creators[])(void) = { create_virtio_vdi_port, create_pci_vdi_port };
> +        for (int i = 0 ; i < sizeof(creators)/sizeof(creators[0]); ++i) {
> +            _vdi_port = creators[i]();
> +            init = _vdi_port->init();
> +            if (init) {
> +                break;
> +            }
> +            delete _vdi_port;
> +        }
> +    }
>   
to my taste, the creator funcs & looping here is cute but over-generic ;)
2 * [new-if(!init)] will be more than enough.
> +    if (!init) {
> +        vd_printf("Failed to create VDIPort instance");
>          CloseHandle(pipe);
>          return false;
>      }
> +    vd_printf("created %s", _vdi_port->name());
>      _events_count = VD_STATIC_EVENTS_COUNT + _vdi_port->get_num_events() + 1 /*for agent*/;
>      _events = new HANDLE[_events_count];
>      _events_vdi_port_base = VD_STATIC_EVENTS_COUNT;
>