Re: [Qemu-devel] [PATCH 2/2] add UUID support and -uuid command line option

[Anthony Liguori]

Gleb Natapov wrote:
> Generate UUID automatically or use UUID provided by user with new -uuid
> command line option (command line and configuration related bits are taken
> from this patch http://www.mail-archive.com/address@hidden/msg14498.html
> by Ryan Harper).
>
> Signed-off-by: Gleb Natapov <gleb <at> qumranet.com>
>   

I think you should split this patch into at least two parts.  The first 
part introduces the -uuid option and the second part plumbs it through 
vmport.

> Index: qemu/hw/vmport.c
> ===================================================================
> --- qemu.orig/hw/vmport.c       2008-06-04 14:17:10.000000000 +0300
> +++ qemu/hw/vmport.c    2008-06-04 15:43:49.000000000 +0300
> @@ -25,13 +25,22 @@
>  #include "isa.h"
>  #include "pc.h"
>  #include "sysemu.h"
> +#ifdef CONFIG_UUID
> +#include <uuid/uuid.h>
> +uuid_t vmport_uuid;
> +extern char *qemu_uuid;
> +#else
> +const char vmport_uuid[16] = "QEMUQEMUQEMUQEMU";
>   

All zeros would be a better choice I think.

> +#endif
>  
>  #define VMPORT_CMD_GETVERSION 0x0a
>  #define VMPORT_CMD_GETRAMSIZE 0x14
> +#define VMPORT_CMD_GETBIOSUUID 0x13
>  
>  #define VMPORT_ENTRIES 0x2c
>  #define VMPORT_MAGIC   0x564D5868
>  
> +
>  typedef struct _VMPortState
>   

Please avoid introducing whitespace.

>  {
>      IOPortReadFunc *func[VMPORT_ENTRIES];
> @@ -93,6 +102,26 @@ static uint32_t vmport_cmd_ram_size(void
>      return ram_size;
>  }
>  
> +static inline uint32_t uuid2reg(const unsigned char *uuid, uint32_t idx)
> +{
> +    int i;
> +    uint32_t reg = 0;
> +
> +    for(i = 0; i < 4; i++)
> +        reg |= (uuid[(idx*4) + i] << (i*8));
>   

An admitted nit, but there should be a space after "for".

> +    return reg;
> +}
> +
> +static uint32_t vmport_cmd_bios_uuid(void *opaque, uint32_t addr)
> +{
> +    CPUState *env = cpu_single_env;
> +    env->regs[R_EBX] = uuid2reg(vmport_uuid, 1);
> +    env->regs[R_ECX] = uuid2reg(vmport_uuid, 2);
> +    env->regs[R_EDX] = uuid2reg(vmport_uuid, 3);
> +    return uuid2reg(vmport_uuid, 0);
> +}
> +
>  void vmport_init(void)
>  {
>      register_ioport_read(0x5658, 1, 4, vmport_ioport_read, &port_state);
> @@ -101,4 +130,13 @@ void vmport_init(void)
>      /* Register some generic port commands */
>      vmport_register(VMPORT_CMD_GETVERSION, vmport_cmd_get_version, NULL);
>      vmport_register(VMPORT_CMD_GETRAMSIZE, vmport_cmd_ram_size, NULL);
> +    vmport_register(VMPORT_CMD_GETBIOSUUID, vmport_cmd_bios_uuid, NULL);
> +
> +#ifdef CONFIG_UUID
> +    if (qemu_uuid != NULL)
> +        if (uuid_parse(qemu_uuid, vmport_uuid) == 0)
> +            return;
> +    fprintf(stderr, "Fail to parse UUID string. Wrong format.\n");
> +    uuid_generate(vmport_uuid);
> +#endif
>   

When nesting if's, you should always use braces in the outer if or just 
combined them with a &&.

Please also introduce a monitor command, "info uuid", so that the user 
can determine what the uuid is.  It seems that you always print "Fail to 
parse UUID string." if qemu_uuid == NULL?  That seems like an 
unnecessary error message since the user didn't do anything wrong by not 
specifying the parameter.

>  }
> Index: qemu/vl.c
> ===================================================================
> --- qemu.orig/vl.c      2008-06-04 15:17:01.000000000 +0300
> +++ qemu/vl.c   2008-06-04 15:44:02.000000000 +0300
> @@ -240,6 +240,9 @@ static CPUState *cur_cpu;
>  static CPUState *next_cpu;
>  static int event_pending = 1;
>  
> +#ifdef CONFIG_UUID
> +const char *qemu_uuid;
> +#endif
>   

This should be unconditional.

>  #define TFR(expr) do { if ((expr) != -1) break; } while (errno == EINTR)
>  
>  /***********************************************************/
> @@ -7265,6 +7268,10 @@ static void help(int exitcode)
>             "-no-shutdown    stop before shutdown\n"
>             "-loadvm [tag|id]  start right away with a saved state (loadvm in 
> monitor)\n"
>            "-vnc display    start a VNC server on display\n"
> +#ifdef CONFIG_UUID
> +           "-uuid %%08x-%%04x-%%04x-%%04x-%%012x\n"
> +           "                specify machine UUID\n"
> +#endif
>   

As should this.  CONFIG_UUID is just the presence of libuuid.

>  #ifndef _WIN32
>            "-daemonize      daemonize QEMU after initializing\n"
>  #endif
> @@ -7379,6 +7386,9 @@ enum {
>      QEMU_OPTION_clock,
>      QEMU_OPTION_startdate,
>      QEMU_OPTION_tb_size,
> +#ifdef CONFIG_UUID
> +    QEMU_OPTION_uuid,
> +#endif
>  };
>  
>  typedef struct QEMUOption {
> @@ -7467,6 +7477,9 @@ const QEMUOption qemu_options[] = {
>  #ifdef CONFIG_CURSES
>      { "curses", 0, QEMU_OPTION_curses },
>  #endif
> +#ifdef CONFIG_UUID
> +    { "uuid", HAS_ARG, QEMU_OPTION_uuid },
> +#endif
>  
>      /* temporary options */
>      { "usb", 0, QEMU_OPTION_usb },
> @@ -8230,6 +8243,11 @@ int main(int argc, char **argv)
>             case QEMU_OPTION_daemonize:
>                 daemonize = 1;
>                 break;
> +#ifdef CONFIG_UUID
> +            case QEMU_OPTION_uuid:
> +                qemu_uuid = optarg;
> +                break;
> +#endif
>             case QEMU_OPTION_option_rom:
>                 if (nb_option_roms >= MAX_OPTION_ROMS) {
>                     fprintf(stderr, "Too many option ROMs\n");
>   

ditto for all of the above.

> Index: qemu/Makefile.target
> ===================================================================
> --- qemu.orig/Makefile.target   2008-06-04 15:26:29.000000000 +0300
> +++ qemu/Makefile.target        2008-06-04 15:28:08.000000000 +0300
> @@ -502,6 +502,10 @@ CPPFLAGS += $(CONFIG_VNC_TLS_CFLAGS)
>  LIBS += $(CONFIG_VNC_TLS_LIBS)
>  endif
>  
> +ifdef CONFIG_UUID
> +LIBS += -luuid
> +endif
> +
>  # SCSI layer
>  OBJS+= lsi53c895a.o esp.o
>  
> Index: qemu/configure
> ===================================================================
> --- qemu.orig/configure 2008-06-04 15:28:18.000000000 +0300
> +++ qemu/configure      2008-06-04 15:55:13.000000000 +0300
> @@ -95,6 +95,7 @@ dsound="no"
>  coreaudio="no"
>  alsa="no"
>  esd="no"
> +uuid="no"
>  fmod="no"
>  fmod_lib=""
>  fmod_inc=""
> @@ -272,6 +273,8 @@ for opt do
>    ;;
>    --enable-fmod) fmod="yes"
>    ;;
> +  --enable-uuid) uuid="yes"
> +  ;;
>   

This is not quite the right way to do this.  uuid="no" signifies that 
libuuid wasn't found.  --enable-uid shouldn't be required, we should 
autodetect it and enable it if it's present.

>    --fmod-lib=*) fmod_lib="$optarg"
>    ;;
>    --fmod-inc=*) fmod_inc="$optarg"
> @@ -423,6 +426,7 @@ echo "  --enable-coreaudio       enable 
>  echo "  --enable-alsa            enable ALSA audio driver"
>  echo "  --enable-esd             enable EsoundD audio driver"
>  echo "  --enable-fmod            enable FMOD audio driver"
> +echo "  --enable-uuid            enable UUID support"
>  echo "  --enable-dsound          enable DirectSound audio driver"
>  echo "  --disable-brlapi         disable BrlAPI"
>  echo "  --disable-vnc-tls        disable TLS encryption for VNC server"
> @@ -774,6 +778,24 @@ EOF
>    fi
>  fi # test "$curses"
>  
> +##########################################
> +# uuid library
> +if test "$uuid" = "yes" ; then
> +  cat > $TMPC << EOF
> +#include <uuid/uuid.h>
> +int main(void) { uuid_t u; return 0; }
> +EOF
> +  if $cc -o $TMPE $TMPC -luuid 2> /dev/null ; then
> +    :
> +  else
> +    echo
> +    echo "Error: Could not find uuid"
> +    echo "Make sure to have the uuid libs and headers installed."
> +    echo
> +    exit 1
> +  fi
> +fi
>   

It is not an error to not find libuuid.  Look at gnutls for an example 
of how probing should be done.

Regards,

Anthony Liguori

>  # Check if tools are available to build documentation.
>  if [ -x "`which texi2html 2>/dev/null`" ] && \
>     [ -x "`which pod2man 2>/dev/null`" ]; then
> @@ -834,6 +856,7 @@ echo "CoreAudio support $coreaudio"
>  echo "ALSA support      $alsa"
>  echo "EsounD support    $esd"
>  echo "DSound support    $dsound"
> +echo "UUID support      $uuid"
>  if test "$fmod" = "yes"; then
>      if test -z $fmod_lib || test -z $fmod_inc; then
>          echo
> @@ -1058,6 +1081,10 @@ if test "$dsound" = "yes" ; then
>    echo "CONFIG_DSOUND=yes" >> $config_mak
>    echo "#define CONFIG_DSOUND 1" >> $config_h
>  fi
> +if test "$uuid" = "yes" ; then
> +  echo "CONFIG_UUID=yes" >> $config_mak
> +  echo "#define CONFIG_UUID 1" >> $config_h
> +fi
>  if test "$fmod" = "yes" ; then
>    echo "CONFIG_FMOD=yes" >> $config_mak
>    echo "CONFIG_FMOD_LIB=$fmod_lib" >> $config_mak
> --
>                         Gleb.
>
>
>