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

Re: [libvirt] [PATCH 01/10] s/qemud/qemu/ in QEMU driver sources



> Change some legacy function names to use 'qemu' as their
> prefix instead of 'qemud' which was a hang over from when
> the QEMU driver ran inside a separate daemon
> 
> Signed-off-by: Daniel P. Berrange <berrange redhat com>
> ---
>  src/qemu/qemu_conf.c         |   4 +-
>  src/qemu/qemu_conf.h         |   4 +-
>  src/qemu/qemu_driver.c       | 541
>  ++++++++++++++++++++++---------------------
>  src/qemu/qemu_monitor_text.c |   6 +-
>  src/qemu/qemu_process.c      |   2 +-
>  5 files changed, 280 insertions(+), 277 deletions(-)

Fairly mechanical.  I'll see if I can spot why there are more
lines added than deleted...

>  
> -static void qemudNotifyLoadDomain(virDomainObjPtr vm, int newVM,
> void *opaque)
> +static void qemuNotifyLoadDomain(virDomainObjPtr vm, int newVM, void
> *opaque)

[Yuck - I'll be glad to get off of this brain-dead web interface,
as it sure mangles patch reviews]

Is it worth a followup patch for lines like this to use our more
common convention of:

static void
qemuNotifyLoadDomain(...)

But that doesn't impact this patch.

> @@ -1130,7 +1130,7 @@ static virDrvOpenStatus qemudOpen(virConnectPtr
> conn,
>      return VIR_DRV_OPEN_SUCCESS;
>  }
>  
> -static int qemudClose(virConnectPtr conn) {
> +static int qemuClose(virConnectPtr conn) {
>      struct qemud_driver *driver = conn->privateData;

As long as we are refactoring code...
Another one bugging me is that we use 'struct qemud_driver *' everywhere
instead of a nicer typedef'd 'qemuDriverPtr'; but that would also be
best done as a separate patch.


> -static int qemudGetMaxVCPUs(virConnectPtr conn ATTRIBUTE_UNUSED,
> const char *type) {
> +static int qemuGetMaxVCPUs(virConnectPtr conn ATTRIBUTE_UNUSED,
> const char *type) {

'{' should be on its own line (saving that for a separate cleanup
patch would be okay though).

>      if (!type)
>          return 16;
>  
> @@ -1246,7 +1246,7 @@ static int qemudGetMaxVCPUs(virConnectPtr conn
> ATTRIBUTE_UNUSED, const char *typ
>  }
>  
>  
> -static char *qemudGetCapabilities(virConnectPtr conn) {
> +static char *qemuGetCapabilities(virConnectPtr conn) {

And again.

> @@ -2587,39 +2587,42 @@ cleanup:
>  }
>  
>  
> -#define QEMUD_SAVE_MAGIC   "LibvirtQemudSave"
> -#define QEMUD_SAVE_PARTIAL "LibvirtQemudPart"
> -#define QEMUD_SAVE_VERSION 2
> +/* It would be nice to replace 'Qemud' with 'Qemu' but
> + * this magic string is ABI, so it can't be changed
> + */
> +#define QEMU_SAVE_MAGIC   "LibvirtQemudSave"
> +#define QEMU_SAVE_PARTIAL "LibvirtQemudPart"
> +#define QEMU_SAVE_VERSION 2

Aha - there's the three added lines.  And I concur; 'virsh restore'
would break if we altered this string.

> -enum qemud_save_formats {
> -    QEMUD_SAVE_FORMAT_RAW = 0,
> -    QEMUD_SAVE_FORMAT_GZIP = 1,
> -    QEMUD_SAVE_FORMAT_BZIP2 = 2,
> +enum qemu_save_formats {

While touching this, should we name it qemuSaveFormats?  (That is,
either here or in a followup, shouldn't enum types be camel-cased?)

> +    QEMU_SAVE_FORMAT_RAW = 0,
> +    QEMU_SAVE_FORMAT_GZIP = 1,
> +    QEMU_SAVE_FORMAT_BZIP2 = 2,
>      /*
>       * Deprecated by xz and never used as part of a release
> -     * QEMUD_SAVE_FORMAT_LZMA
> +     * QEMU_SAVE_FORMAT_LZMA
>       */

At this point, we could delete this comment altogether.

> @@ -2628,7 +2631,7 @@ struct qemud_save_header {
>  };
>  
>  static inline void
> -bswap_header(struct qemud_save_header *hdr) {
> +bswap_header(struct qemu_save_header *hdr) {

Should we be renaming this helper function into
qemuBswapHeader?

>      hdr->version = bswap_32(hdr->version);
>      hdr->xml_len = bswap_32(hdr->xml_len);
>      hdr->was_running = bswap_32(hdr->was_running);
> @@ -2639,7 +2642,7 @@ bswap_header(struct qemud_save_header *hdr) {
>  /* return -errno on failure, or 0 on success */
>  static int
>  qemuDomainSaveHeader(int fd, const char *path, const char *xml,
> -                     struct qemud_save_header *header)
> +                     struct qemu_save_header *header)

Another annoying struct name, where a typedef'd qemuSaveHeaderPtr
would be nicer.  But again, save it for a followup patch.

ACK.  This will cause conflicts to any outstanding patches or to
backports of patches made after this point, but I think we can
live with that; besides, we seem to be past the worst of the
backport churn (observent list readers can probably pinpoint
cycles of major backport efforts vs. major feature development).


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