From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1K3tq5-0005TW-K4 for qemu-devel@nongnu.org; Wed, 04 Jun 2008 10:19:45 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1K3tq3-0005Rm-PI for qemu-devel@nongnu.org; Wed, 04 Jun 2008 10:19:44 -0400 Received: from [199.232.76.173] (port=38603 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1K3tq3-0005RT-8f for qemu-devel@nongnu.org; Wed, 04 Jun 2008 10:19:43 -0400 Received: from an-out-0708.google.com ([209.85.132.249]:28876) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1K3tq2-0006b4-21 for qemu-devel@nongnu.org; Wed, 04 Jun 2008 10:19:42 -0400 Received: by an-out-0708.google.com with SMTP id d18so31870and.130 for ; Wed, 04 Jun 2008 07:19:41 -0700 (PDT) Message-ID: <4846A46F.3050705@codemonkey.ws> Date: Wed, 04 Jun 2008 09:19:27 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 2/2] add UUID support and -uuid command line option References: <20080604132510.GC31694@minantech.com> In-Reply-To: <20080604132510.GC31694@minantech.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org 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/qemu-devel@nongnu.org/msg14498.html > by Ryan Harper). > > Signed-off-by: Gleb Natapov 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_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 > +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. > > >