[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] [QEMU 3/3] the qemu driver & daemon



On Mon, Aug 28, 2006 at 10:06:03AM +0200, Karel Zak wrote:
> On Mon, Aug 28, 2006 at 12:18:52AM +0100, Daniel P. Berrange wrote:
> > Attached is the patch for the qemu driver & daemon process
> 
>  Cool pathes!
> 
> 
>  On first look (I hope I'm not too pedantic:-):
> 
> 
> > +static int qemudParseUUID(const char *uuid,
> > +			  unsigned char *rawuuid) {
> 
>  We have virParseUUID() in xml.c, I think we should maintain more than
>  one version of same code.

Yes, be nice to re-factor helper methods into a dir which can be shared
between the daemon & libvirt

> > +  if (!(*argv = malloc(sizeof(char *) * *argc)))
> > +    return -1;
> 
>  Please,  virQemuError(VIR_ERR_NO_MEMORY ....) (same or calloc())

I forgot to mention in my posting - I simply return a generic -1, internal
error everywhere in the code. I'm planning to go through it again to add
in explicit, more useful error codes.

> 
> > +  printf("Load VM %s\n", file);
> > +  if (!(xml = xmlReadDoc(BAD_CAST doc, file ? file : "domain.xml", NULL,
> > +                         XML_PARSE_NOENT | XML_PARSE_NONET |
> > +                         XML_PARSE_NOERROR | XML_PARSE_NOWARNING))) {
> > +    printf("malformed\n");
> > +    return NULL;
> > +  }
> 
>  printf()... it seems you forgot there your debug messages ;-)

yes, quite a few of those need taking out - to be done at same time as
error codes are added

> > +static void qemudLoadConfig(struct qemud_server *server,
> > +			    const char *file) {
> > +  FILE *fh;
> > +  struct stat st;
> > +  struct qemud_vm *vm;
> > +  char xml[QEMUD_MAX_XML_LEN];
> > +  int ret;
> > +
> > +  if (!(fh = fopen(file, "r"))) {
> > +    return;
> > +  }
> 
>  No error message?

This is run during initialization when it scans all config files in user's
$HOME/.qemu.d directory. Since the daemon is already running in the background
STDOUT & STDERR are already pointing to /dev/null. Of course during testing I
run it in foreground (hence the printf()'s, but ordinarily we wouldn't see 
this. I think we need some very basically logging code to output issues like
this to $HOME/.qemud.d/daemon.log

> > +static
> > +int qemudBufferAdd(struct qemudBuffer *buf, const char *str) {
> > +  int need = strlen(str);
> > +
> > +static
> > +int qemudBufferPrintf(struct qemudBuffer *buf,
> > +		      const char *format, ...) {
> 
>  Duplicate code?

Yes - needs re-factoring.

> > +  if (qemudBufferPrintf(&buf, "  <uuid>%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x</uuid>\n",
> > +			uuid[0], uuid[1], uuid[2], uuid[3],
> > +			uuid[4], uuid[5], uuid[6], uuid[7],
> > +			uuid[8], uuid[9], uuid[10], uuid[11],
> > +			uuid[12], uuid[13], uuid[14], uuid[15]) < 0)
> 
>  It also seems there is again duplicate code do in test.c and xml.c.

Yes - needs re-factoring.

> > +      qemudBufferPrintf(&buf, "    <graphics type='vnc'/>\n");
> 
>  no format args --> BufferAdd()

Opps, misssed that!

Dan
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]